Skip to content
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

[5.8] Fix pluralizing model names ending in words with an irregular plural #26421

Merged
merged 5 commits into from
Nov 9, 2018
Merged

Conversation

SjorsO
Copy link
Contributor

@SjorsO SjorsO commented Nov 7, 2018

Currently, running artisan make:model RealHuman -m gives you a migration called 2018_11_07_133204_create_real_humen_table and a table called real_humen. This happens because it tries to pluralize the word RealHuman, and since that word isn't in the list of irregular plurals the Doctrine Inflector applies the normal (in this case incorrect) rules to pluralize the word.

This PR adds a Str::pluralStudly helper that pluralizes only the last word of a studly caps case string. This helper is used throughout the framework to correctly generate plurals of model names.

The old Str::plural is still being used in the morphToMany method. If this PR gets merged that should also be fixed and tested.

Edit:
turns out someone had this issue before: doctrine/inflector#35

@taylorotwell
Copy link
Member

Part of me thinks we may be putting quite a bit of logic into determining the table name of a very edge-case model name?

@SjorsO
Copy link
Contributor Author

SjorsO commented Nov 7, 2018

Any model name consisting of more than one word, ending in a word with an irregular plural currently doesn't work correctly. Three lines of tested code seems like a reasonable fix for this problem.

I ran into this problem last week when i tried to create a model called VerifiedHuman to store the session ids of guests that had passed a captcha.

@taylorotwell
Copy link
Member

Is there a reason you didn't want to include the morphToMany fix in this PR as well? Thanks.

@SjorsO
Copy link
Contributor Author

SjorsO commented Nov 7, 2018

I wanted to get some feedback on the PR before doing the effort of writing a failing test for a morphToMany relationship. I'll make some time tomorrow and write one.

@SjorsO
Copy link
Contributor Author

SjorsO commented Nov 8, 2018

I've added a test and a fix for morhpToMany relationships.

It would be a little bit cleaner with a Str::pluralSnake helper, but since this is the only place in the framework where we have to pluralize a snake case string i didn't create a helper for it.


$lastWord = array_pop($words);

$table = implode('', $words).Str::plural($lastWord);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you build the table with an empty string implode and not underscores?

Copy link
Contributor Author

@SjorsO SjorsO Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PREG_SPLIT_DELIM_CAPTURE makes it also capture the delimiter (in this case the underscore), so it can be imploded without a delimiter to rebuild the original string. I copied this code from the Str::pluralStudly method, that's why its like this. Doing it this way is necessary when exploding studly case strings, it isn't really necessary to do it this way for snake case strings.

It could be changed to more familiar php like this:

$words = explode('_', $name);

$words[count($words) - 1] = Str::plural($words[count($words) - 1]);

$table = implode('_', $words);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants