[PropertyInfo] Implement Naming Strategy for Properties #16889
Comments
|
It's definitely a bug. The first letter must be lower cased. About the naming strategy, why not adding support for snake_case because as you pointed out it exists in PropertyAccess. But IMO it's better to have be coherent between these 2 components: support only camelCase/snake_case for both or a support for custom strategies for both. |
|
However, a basic property name transformer might be a good idea to maintain consistency across extractors. |
|
Currently doctrine does not fully support snake-case properties: doctrine/collections#29 |
|
Good point. It's probably best that the component does not take on the role of dictating naming conventions, especially since PSRs actively avoid taking a stance on such issues. How about implementing a closest-match property name transformer to match identifiers to the correct class properties/methods?
To return a property identifier, return class properties as-is and lowercasing the first letter of those determined from class methods. There won't be any issue of which naming convention ( Trying to support naming strategies would result in compatibility and support nightmares. There are just too many possible ways for it to go wrong, especially if a mix of naming conventions were used within the same class. interface PropertyNameTransformerInterface
{
/**
* @param string $property
* @return string
*/
public function sourceToIdentifier($property);
/**
* @param string $name
* @return string
*/
public function identifierToSource($name);
}Only issue is how to change the public API of the extractors to ensure each one gets this new property name transformer. |
|
Edit; the more I think about this the more I want to just lowercase the first letter... The PropertyAccess component already maps identifier strings to the correct property/method, why include it in PropertyInfo which is such a small component in comparison? It's an unnecessary overhead, especially where multiple properties/methods exist with only capitalisation differences. Compatibility with the PropertyAccess component needs to improve in the future but I don't think a naming strategy is the way to go IMO.
Simple and effective. Thoughts? |
…actor (zanderbaldwin) This PR was squashed before being merged into the 2.8 branch (closes #16911). Discussion ---------- [PropertyInfo] Update List Information from ReflectionExtractor | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16889 | License | MIT | Doc PR | symfony/symfony-docs#5974 Unless a property with the same casing exists, lowercase the first letter of a property name extracted from a method. From what I understand, we don't actually need to support `snake_case` at all in the PropertyInfo component. I cannot think of any use-case where PropertyAccess would be used to get/set a property value *before* using PropertyInfo to find out information about the property and the values it supports. Since PropertyInfo supports the discovery of property names, which can then be used as identifiers in PropertyAccess, there should be no need to support a naming strategy to map property identifiers from one naming convention to another. --- Running `$reflectionExtractor->getProperties($class)` with the following classes: ```php class X { public $a; public $b; public function getA() {} } // Result: array('a', 'b'); ``` ```php class Y { public $A; public $b; public function setA() {} } // Result: array('A', 'b'); ``` ```php class Y { public $username; protected $emailAddress; public $password; public function getEmailAddress() {} public function isActive() {} } // Result: array('username', 'emailAddress', 'password', 'active'); ``` Commits ------- b2da76c [PropertyInfo] Update List Information from ReflectionExtractor
… isReadable() when checking snake_case properties (jbtronics) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [PropertyInfo] Make isWriteable() more consistent with isReadable() when checking snake_case properties | 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: ```php 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 Commits ------- 7c9e6bc [PropertyInfo] Make isWriteable() more consistent with isReadable() when checking snake_case properties
The
ReflectionExtractorsees a public property as separate from its accessor/mutator methods. For example:The easiest fix would be to lowercase the first letter of the property name determined from a method, though this would cause problems with properties that actually do start with an uppercase letter.
Keeping in mind that it's likely that this component will be used in conjunction with the PropertyAccess component, would it be suitable for a naming strategy to be added (the most obvious being the underscore naming strategy used in both PropertyAccess and Doctrine)?
/cc @dunglas
The text was updated successfully, but these errors were encountered: