Return points, spelling and dead code #304
Conversation
|
@calcinai please check the PR build for test failures |
|
Oh no! Do you think the tests should actually be updated in this case? From my understanding that function is designed to throw an exception or silently return? |
|
Have updated the test, I can only assume that this is desired behaviour - it would be an incredibly strange case if someone was actually looking for nulls in this case.. |
|
@calcinai thanks for this work. While the updating some return values to match doc blocks makes a lot of sense, I want to caution that there are some BC implications to changing return values. My assessment is that no external uses would change so I think we are good, but I would like a second opinion. @FlorianSW @mirfilip @thewilkybarkid @narcoticfresh do you see any BC issues with this? Rolling 4.0.0 is not desireable to me at the present time. |
| */ | ||
| public function confirmMediaType($uriRetriever, $uri) | ||
| { | ||
| $contentType = $uriRetriever->getContentType(); | ||
|
|
||
| if (is_null($contentType)) { | ||
| // Well, we didn't get an invalid one | ||
| return; | ||
| return true; |
There was a problem hiding this comment.
I'm not sure, how many users uses this method, but changing from null to a boolean value could be problematic, especially in code something like:
if ( confirmMediaType( $uriRetriever, $uri ) ) {
// the code that runs when true
} else {
// the former "null" value-code
}
It now would break, as null evaluates to false, and not to true. So in some cases, this could be a problem. However, the new values makes sense, too. I'm really not sure :(
There was a problem hiding this comment.
Given that (in master) the function could return true when it's a schema from json-schema.org, and null if it is explicitly valid, it just isn't seem all that logical. The only usage internally just relies on it halting with an exception if it's invalid.
Is it even feasible that someone has written code to handle the different return types? If you're using the library for validation, it's going to be called again anyway and you won't have any control of the result..
That said, maybe there is an argument for leaving it as it is, because the return value is basically irrelevant anyway!
There was a problem hiding this comment.
@FlorianSW thank you so much for your feedback. This is exactly my concern.
As you pointed out, null is falsy. Perhaps returning false here is the solution as it would be compliant with the bool return type and also be compatible with previous control-flow usage, although that would not be true for === null or is_null comparisons.
There was a problem hiding this comment.
Moving forward with this, should I just revert the section in question?
There was a problem hiding this comment.
Maintaining BC is preferred.
|
+1 |
Replaces #302