Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
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
nicolas-grekas commentedJan 9, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 withstring
direclty like in the existing component ?
Also, we cannot deprecate a stable component in favor of an experimental one IMO.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
fancyweb commentedMar 3, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
to90fa098
CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
// 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 changedreturn $string
toreturn [$string]
to always return an array.
fancyweb commentedApr 30, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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/AWith#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.
Uh oh!
There was an error while loading.Please reload this page.
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):