-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[PropertyAccess] StringUtil miss-singularize some "ee" words #13714
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
| Q | A | ------------- | --- | Bug fix? | [yes] | New feature? | [no] | BC breaks? | [no] | Deprecations? | [no] | Tests pass? | [yes] | Fixed tickets | [no] | License | MIT | Doc PR | [n/a] Fixes misspelling of words like: - street => stroot - fleet => floot Is this proper way to fix this? Can add tests if required.
@@ -197,7 +202,7 @@ public static function singularify($plural) | |||
} | |||
|
|||
// Convert teeth to tooth, feet to foot | |||
if (false !== ($pos = strpos($plural, 'ee')) && strlen($plural) > 3) { | |||
if (false !== ($pos = strpos($plural, 'ee')) && strlen($plural) > 3 ! in_array($plural, self::$allowedEes)) { |
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.
this is invalid code
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.
and your in_array check is broken because it is case sensitive so you don't always match it.
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.
What would you suggest then?
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.
@TomasVotruba for the second comment ? Use an array with lowercase strings in it, and apply strtolower on the plural before the check
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.
@stof Yes, sorry, I'll be more specific next time.
Thanks, I'll implement the changes.
On second thought, maybe it would be easier to specify all "teeth, feet"-kind of words, than to allow all that include "ee" + exclude others. What do you think?
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.
By the way, you forgot &&
yes, tests are needed |
Wouldn't it be cheeper to list the expections; |
@SpacePossum a PR related to the word |
@SpacePossum I agree with you: #13714 (comment) @jakzal I can prepare PR with tests to 2.3 branch. What do you think about "convert only" over "convert all with 'ee' excluding"? |
Not relevant anymore. |
Fixes misspelling of words like:
See exception:
Is this proper way to fix this?
Can add tests if required.