-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Inflector][String] Move Inflector in String #35092
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
24588ce
to
2ddd0ef
Compare
2ddd0ef
to
e703077
Compare
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.
I like the idea
Me too :)
I think this is the way to go personally. |
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.
should we really remove any API dealing with string
direclty like in the existing component ?
Also, we cannot deprecate a stable component in favor of an experimental one IMO.
Note that these 2 TODOs look impossible to me as long as String is experimental (we should not make PropertyAccess and PropertyInfo stable components depend on an experimental one. Also, given the performance sensitivity of these 2 components, I would like to see benchmarks of using your new Inflector vs using the existing one relying on PHP strings. Also, what is the migration plan ? The current Inflector has a non-configurable static API. Your new inflector would require injecting an inflector in PropertyAccess and PropertyInfo. But changing the inflection rules used to access properties can become quite unexpected (if your project changes them, it might break all forms shipped by bundles where they expect PropertyAccess to work based on the core rules). |
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.
Note that these 2 TODOs look impossible to me as long as String is experimental
true, and there is no reason to make the component experimental anymore to me.
Also, given the performance sensitivity of these 2 components, I would like to see benchmarks of using your new Inflector vs using the existing one relying on PHP strings.
the implementation should use native string functionsn, that would fix most of the perf considerations.
Also, what is the migration plan ? The current Inflector has a non-configurable static API. Your new inflector would require injecting an inflector in PropertyAccess and PropertyInfo. But changing the inflection rules used to access properties can become quite unexpected (if your project changes them, it might break all forms shipped by bundles where they expect PropertyAccess to work based on the core rules).
This happens with all services, so we don't have to care much here. Also the current consumers would just hardcode instantiating the new inflector if we really don't want that to ever happen.
From my benchmark, using the String component makes
Since what we have is an English inflector, do you mean we should directly return the passed string suffixed with |
80f7d4c
to
90fa098
Compare
|
||
// Check if the word is one which is not inflected, return early if so | ||
if (\in_array($lowerPluralRev, self::$uninflected, true)) { | ||
return [$plural]; |
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.
Basically I copy pasted the whole code and I only changed return $string
to return [$string]
to always return an array.
I reworked on this with a simple approach. We just copy paste the existing code in the String component. We add an interface. The only code change we make is that singularize and pluralize always return an array of strings. I deprecated the Inflector component. The deprecated Inflector component uses the String component. The PropertyInfo ReflectionExtractor class can now receive an InflectorInterface in its constructor. By default, it instantiates an EnglishInflector so there is no BC break. I guess we don't need to make this more complicated. Thanks to @nicolas-grekas for his suggestions btw. |
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.
LGTM, just minor details.
Thank you @fancyweb. |
…tring (derrabus) This PR was merged into the 5.1-dev branch. Discussion ---------- [String] Move Inflector's polyfill-ctype dependency to String | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | N/A | License | MIT | Doc PR | N/A With #35092, the inflector implementation was moved to the string component, including all calls to `ext-ctype`. This is why I think the dependency on the corresponding polyfill should be moved as well, which is what this PR does. Commits ------- de960b8 [String] Move Inflector's polyfill-ctype dependency to String.
This PR was merged into the 6.0 branch. Discussion ---------- [Inflector] Remove the component | Q | A | ------------- | --- | Branch? | 6.0 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Ref #35092 Commits ------- fc9683b [Inflector] Remove the component
Needs #35091.
Should we have a standalone inflector (like the Slugger) or 2 new methods (pluralize and singularize) on the AbstractString class? I implemented both but since we only handle English I finally preferred the first one.
TODO (after the "move" is OK):