Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:masterfromfancyweb:string-inflector
May 5, 2020

Conversation

fancyweb
Copy link
Contributor

@fancywebfancyweb commentedDec 23, 2019
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?yes
Ticketshttps://github.com/orgs/symfony/projects/1#card-30499514
LicenseMIT
Doc PR-

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):

  • Deprecate the Inflector component
  • Use the String inflector in Symfony's code

ro0NL and azjezz reacted with rocket emoji
@nicolas-grekasnicolas-grekas added this to thenext milestoneDec 26, 2019
Copy link
Member

@fabpotfabpot left a 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
Copy link
Member

nicolas-grekas commentedJan 9, 2020
edited
Loading

Me too :)

standalone inflector (like the Slugger)

I think this is the way to go personally.

Copy link
Member

@stofstof left a 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.

@stof
Copy link
Member

stof commentedJan 9, 2020

  • Deprecate the Inflector component
  • Use the String inflector in Symfony's code

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.
As all the English inflection rules are pure ASCII, I don't see any benefit for using the String abstraction (which probably adds overhead compared to native 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).

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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.

@fancyweb
Copy link
ContributorAuthor

fancyweb commentedMar 3, 2020
edited
Loading

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.

From my benchmark, using the String component makessingularize between 5 to 7 times slower.

As all the English inflection rules are pure ASCII, I don't see any benefit for using the String abstraction (which probably adds overhead compared to native strings).

the implementation should use native string functionsn, that would fix most of the perf considerations.

Since what we have is an English inflector, do you mean we should directly return the passed string suffixed withs (for pluralize as example) when it does not match[a-zA-Z]+ and keep the current code (using native string functions) for the rest?

@fancywebfancywebforce-pushed thestring-inflector branch 3 times, most recently from80f7d4c to90fa098CompareApril 30, 2020 13:23

// Check if the word is one which is not inflected, return early if so
if (\in_array($lowerPluralRev, self::$uninflected, true)) {
return [$plural];
Copy link
ContributorAuthor

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.

@fancywebfancyweb marked this pull request as ready for reviewApril 30, 2020 13:27
@fancyweb
Copy link
ContributorAuthor

fancyweb commentedApr 30, 2020
edited
Loading

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.

@fancywebfancyweb changed the title[String] Move Inflector in String[Inflector][String] Move Inflector in StringApr 30, 2020
Copy link
Member

@nicolas-grekasnicolas-grekas left a 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.

@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpot
Copy link
Member

Thank you@fancyweb.

@fabpotfabpot merged commit3e737ec intosymfony:masterMay 5, 2020
@fancywebfancyweb deleted the string-inflector branchMay 5, 2020 06:53
@fabpotfabpot mentioned this pull requestMay 5, 2020
nicolas-grekas added a commit that referenced this pull requestMay 16, 2020
…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.
fabpot added a commit that referenced this pull requestMay 27, 2021
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#35092Commits-------fc9683b [Inflector] Remove the component
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof left review comments

@fabpotfabpotfabpot approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

5 participants
@fancyweb@nicolas-grekas@stof@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp