-
Notifications
You must be signed in to change notification settings - Fork 46
Fixed typo in method name #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks correct to me.
symfony tends to put deprecated warning triggers into deprecated code to make people without code quality tools notify the deprecation, but i am not sure if we want to do the same in our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct. But I don’t see any reason for it. We just introduce one more method and a deprecation layer..
what do we actually win? How does this help the user?
It helps the user because they will type in uri instead of url and get a fatal error. I say this because I've done it. ;) |
Friendly ping. I still think this should be changed. ;) |
@Nyholm wdyt? to me it makes sense. i would also be fine leaving the old method name without deprecating, and a phpdoc saying it is an alias for the findUriFactory - this is discovery, not costing us anything to have that alias. |
Again. I agree that this is correct. But does it really matter? I dont think so. I vote for closing this PR. Im happy to add some docs/comment to explain the mistake in naming. |
i think the principle of least astonishment does have value here. the cost of having two methods, the legacy one and the expected "correct" one is easier to handle and more user friendly than confusing people with a warning in a documentation that a method is not named as you'd expect - many won't look at that documentation anyways but directly interact with the code |
So, what's the decision here? |
@Nyholm and me seem to disagree :-/ i'd rather fix this (while keeping BC, of course) than not fix but document a naming mistake. |
I'm in favor of @dbu here, it does not really matter like @Nyholm say but doing this change does not break anything, and i ever doubt there will be a 2.0 for this package, Even if there is and this change is too big to handle (we can expect anything) we can always keep the old method as a deprecated alias. |
URI Discovery was replaced by PSR-17 discovery, but the method was named incorrectly. It returns a
UriFactoryInterface
. Moreover, the old discovery class was alsoUri
, so the previous new name is not only wrong, but also not consistent.