Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PropertyInfo] Make isWriteable() more consistent with isReadable() when checking snake_case properties#51697
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
nicolas-grekas merged 1 commit intosymfony:6.4fromjbtronics:property_info_underscore_consistency_fixSep 29, 2023
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
8cc6f97 to97134b9Compare…hen checking snake_case properties
4af0726 to7c9e6bcComparenicolas-grekas approved these changesSep 29, 2023
Member
nicolas-grekas commentedSep 29, 2023
Thank you@jbtronics. |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently PropertyInfo is a bit inconsistent in the behavior of isWriteable() and isReadable() when using it with properties in snake_case format, which are only accessible via a getter and setter. To be readable the getter function has to be in camel_case (e.g.
getSnakeCase()), while the setter function has to remain in the snake case format (e.g.setSnake_case()).In this example class:
the property
snake_caseis considered readable like you would expect, but not writeable, even though the property has a useable setter and the value can be actually be modified fine by something like the PropertyAccess component.To make the property actually considered writeable, the setter would need to be named
setSnake_case, which is pretty inconsistent with the behavior of isReadable and makes it very hard to use this component on snake_case properties.This inconsistencies are caused by the fact, that
isReadablein ReflectionExtractor uses thegetReadInfo()function which does a camelization of the property name internally, while theisWriteable()function uses thegetMutatorMethod()function which just perform a capitalization of the first letter.This PR fixes this inconsistencies between isReadable() and isWriteable() by allowing to use a camelCase style setter to be considered writeable, as this is much more common then to use the mix of snake and camelCase currently required.
The getWriteInfo() function is not useable here, because it needs both a add and remove Function on collection setters to give proper infos, while the current
isWriteable()implementation considers a class with just a add or a remove function as writeable. Therefore the property name just gets camelized before being feed into thegetMutatorMethod(), which gives the desired result.To maximize backwards compatibility, if no camelCase style setter is found, it is still checked for a snake_case setter, so that classes having these, are still considered writeable after the change.
The current behavior is causing some inconsistencies in higher level libraries, which rely on this component. In API Platform for example properties in snake_case are considered read only even though they have a (camel case) setter, because of this. See issue:api-platform/core#5641 andapi-platform/core#1554