Skip to content
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

[PropertyInfo] Make isWriteable() more consistent with isReadable() when checking snake_case properties #51697

Merged

Conversation

jbtronics
Copy link
Contributor

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Related with issues #29933 and #16889
License MIT
Doc PR

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:

class Dummy {
  private string $snake_case;

  public function getSnakeCase(): string
  { }

  public function setSnakeCase(string $snake_case): void
  { }
 
}

the property snake_case is 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 isReadable in ReflectionExtractor uses the getReadInfo() function which does a camelization of the property name internally, while the isWriteable() function uses the getMutatorMethod() 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 the getMutatorMethod(), 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 and api-platform/core#1554

@jbtronics jbtronics force-pushed the property_info_underscore_consistency_fix branch from 8cc6f97 to 97134b9 Compare September 20, 2023 09:33
@nicolas-grekas nicolas-grekas added this to the 6.4 milestone Sep 25, 2023
@nicolas-grekas nicolas-grekas force-pushed the property_info_underscore_consistency_fix branch from 4af0726 to 7c9e6bc Compare September 29, 2023 17:23
@nicolas-grekas
Copy link
Member

Thank you @jbtronics.

@nicolas-grekas nicolas-grekas merged commit 83910a9 into symfony:6.4 Sep 29, 2023
7 of 9 checks passed
This was referenced Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants