Pass roots to FileSystemLocator during construction (1.0 branch) #932
Conversation
rpkamp
commented
May 13, 2017
rpkamp
commented
| Q | A |
|---|---|
| Branch? | 1.0 |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | See #930 |
| License | MIT |
There was a problem hiding this comment.
Looks like you forgot some short array syntax within the test cases and possibly other CS errors; please run PHP CS Fixer to ensure your code meets the required code style.
./bin/php-cs-fixer fix .
There was a problem hiding this comment.
Also, looks like there are errors on lower PHP and Symfony versions.
robfrawley
added
Attn: Deprecation
| throw new InvalidArgumentException('One or more data root paths must be specified.'); | ||
| } | ||
| if (is_array($locatorOrDataRoots)) { // BC | ||
| if (func_num_args() === 4 && func_get_arg(3) instanceof LocatorInterface) { |
There was a problem hiding this comment.
I've been wondering about this one. To me this seems like a BC break, meaning this cannot be in a 1.x version, but according to semver should bump the version to 2.0.
On the other hand this is an internal class and people aren't supposed to create it themselves, so maybe it's okay.
There was a problem hiding this comment.
Here is my take:
- The constructor never actually had a fourth parameter in the signature (it was commented out), so those extending this class won't have their code break.
- According to semvar we just need to increment the minor version to introduce a deprecation.
- As long as you haven't broken any of the following constructor usage, we haven't broken any code:
MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, $dataRoots(where $dataRoots can be a single string root or an array of roots)MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, $dataRoots, LocatorInterface $locator(where $dataRoots can be a single string root or an array of roots)MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, LocatorInterface $locator
- Ensure your changes to the tests didn't remove any of the aforementioned signature checks, as well.
There was a problem hiding this comment.
Do you agree with my assertions @antoligy?
There was a problem hiding this comment.
MimeTypeGuesserInterface $mimeGuesser, ExtensionGuesserInterface $extensionGuesser, $dataRoots, LocatorInterface $locator (where $dataRoots can be a single string root or an array of roots)
exactly that use case is broken in these proposed changes, because you can't pass data roots to the LocatorInterface anymore, since it's a constructor argument and cannot (and should not, imho) be changed at runtime.
Maybe it's better to just add deprecation notices to 1.0 but don't actually change anything, and then use 2.0 to actually perform the change?
There was a problem hiding this comment.
Sorry, I didn't even realize you'd removed setOptions from the Loader; that needs to stay with a deprecation notice. Then, in the Locator constructor, you should pass the array to setOptions, as before. As far as the other changes, of defaulting to 2.0 usage, that's fine, as long as we support the old use cases.
There was a problem hiding this comment.
As far as the other changes, of defaulting to 2.0 usage, that's fine, as long as we support the old use cases.
Do you mean the setOptions must stay in 2.0? If so this PR is a step backward because it only adds complexity instead of making things more simple. I was hoping for removing setOptions altogether in 2.0 since it's basically a bundle-internal class and 2.0 already indicates there will be BC breaks.
There was a problem hiding this comment.
We could then change this PR to only generate E_DEPRECATED warnings in 1.0, without actually changing anything.
There was a problem hiding this comment.
No, I was saying to keep setOptions for 1.x with a deprecation added; it can be removed for 2.x, of course.
There was a problem hiding this comment.
In that case I'll close this PR and create a new one with deprecation warnings only 😃
lsmith77
removed
the
State: Reviewing
