uninflected words in a patch version

[prwnr]

I would like to raise a problem here that adding a bunch of words to the "uninflected" list is kind of odd to include in a patch version.

The problem here, in my case, is that the Laravel framework depends on this package for automatically generating database table names. Now, after the 2.0.7 patch, some words (like permission) are no longer pluralized and no longer match the already created table names that previous versions provided.

There is a workaround on my side, and I will be applying it. Nonetheless, I think it's worth mentioning that this change may raise problems on the end-user side because of it being a patch.

[NathanAston]

Confirming this, also with a model called Permission (which would resolve to a table called permissions).

That aside, I'm not particularly sure I agree with permission being considered an uncountable word.

[greg0ire]

Thanks for this report and your remarks. Let's try to get something actionable out of this.

I would like to raise a problem here that adding a bunch of words to the "uninflected" list is kind of odd to include in a patch version.

kind of odd? Unless you know a large part of the users use the output of this package to generate database table names, it does not look that odd to me: the method outputs something that is clearly wrong, we fix it, that does sound like a patch.

The problem here, in my case, is that the Laravel framework depends on this package for automatically generating database table names. Now, after the 2.0.7 patch, some words (like permission) are no longer pluralized and no longer match the already created table names that previous versions provided.

I must admit I don't have in mind all the uses cases for this package, maybe that's not only an issue for Laravel. It might be worth documenting how this package is used inside and outside of Doctrine.

There is a workaround on my side, and I will be applying it. Nonetheless, I think it's worth mentioning that this change may raise problems on the end-user side because of it being a patch.

I definitely see how this can be an issue. In fact, it reminds me a bit of another Doctrine repository: doctrine/coding-standard. We've had similar issues with that repository many times, to the point that I clarified what we should pay attention to besides simply applying semver: https://github.com/doctrine/coding-standard/pull/285/files

Maybe something similar should be done for that repository as well? Another way to address the issue you're having might be to rework Laravel so that this method is called to generate a table name, but only once, meaning the table name would get stored in a configuration file, and then never change. That way, even if we release a major version of this library, you don't have to do a migration. I'm not sure if it's the right call but it might be worth considering.

I'm not particularly sure I agree with permission being considered an uncountable word.

Maybe it depends of the context? Can somebody research/corroborate that?

[greg0ire]

Looks like "permission" is not the only questionable fix, see #222

[limenet]

Maybe it depends of the context? Can somebody research/corroborate that?

The entry for "permission" in the Oxford Learner's Dictionary says

[countable, usually plural] an official written statement allowing somebody to do something

Similarly for "fruit"

[countable, uncountable] the part of a plant that consists of one or more seeds and a soft inner part, can be eaten as food and usually tastes sweet

and "experience"

[countable] an event or activity that affects you in some way

Without going through every entry in #216 and checking it against Oxford, there are a few more such as "accommodation", "behavior", "business", "cake", "coffee", "damage" etc. that have at least one sense in which it is countable.

[greg0ire]

Can one of you go through the whole list of words introduced in #216 and send a PR that reverts anything dubious? Otherwise I think I will just revert it.

[driesvints]

ind of odd? Unless you know a large part of the users use the output of this package to generate database table names, it does not look that odd to me: the method outputs something that is clearly wrong, we fix it, that does sound like a patch.

I can see your reasoning here. All depends on how you consider the usage of this library. If this library is intended for text generation and not anything key-based then it's indeed fine to do it in a patch release. Otherwise if you consider key-generation to be a usage then a major version would be needed.

I must admit I don't have in mind all the uses cases for this package, maybe that's not only an issue for Laravel. It might be worth documenting how this package is used inside and outside of Doctrine.

I also think that clearly defining how this package should be used would help. Right now, I don't feel the readme is doing that: https://github.com/doctrine/inflector

Doctrine Inflector is a small library that can perform string manipulations with regard to uppercase/lowercase and singular/plural forms of words.

Another way to address the issue you're having might be to rework Laravel so that this method is called to generate a table name, but only once, meaning the table name would get stored in a configuration file, and then never change.

This is unfortunately unpractical and would require a major rewrite in how we approach table generation. I don't think we will do that anytime soon.

All in all I think that depending on how you intended to let this library be used we need to do A or B:

• A: doctrine/inflector is intended for text generation only and not key-based values. In this case I think we ourselves would need to step away from this library and maintain our own list of words (would need to discuss this with the team)
• B: doctrine/inflector is intended for key-based values as well: the PR that caused this should be reverted and only be done in a major version

Hope that's clear 🙂

[greg0ire]

Very clear. So far there does not seem to be any complaints from the Symfony/Doctrine ecosystem, but I think I'll try to research how it's used anyway. Feel free to beat me to it, anybody!

[driesvints]

@greg0ire ok. In the meantime could we make a decision about my open PR? We need to act fast on it on our side to prevent more PRs from failing. Otherwise I'll need to patch Laravel's test suite itself for now.

[derrabus]

Another way to address the issue you're having might be to rework Laravel so that this method is called to generate a table name, but only once, meaning the table name would get stored in a configuration file, and then never change.

This is unfortunately unpractical and would require a major rewrite in how we approach table generation. I don't think we will do that anytime soon.

But that basically means that we would need to issue a major version bump everytime we fix a pluralization rule, doesn't it?

[driesvints]

But that basically means that we would need to issue a major version bump everytime we fix a pluralization rule, doesn't it?

Basically yes, I think you need to decide for yourself if the outcome of pluralization is part of the public API or not. If you think it's not then we need to re-evaluate how we use this library.

[driesvints]

Also want to make it clear that we've been using doctrine/inflector for years without any issue. I think we one time had a similar issue which was fixed right away.

[greg0ire]

@driesvints I don't think we will merge your open PR, unless you add more words as suggested by #221 (comment)

Let me know if you don't plan to do so, and I will just revert #216 entirely so that you are unblocked. We can still define a clear policy later.

[Tofandel]

I can confirm that this should not be a patch version, I had the issue while upgrading from laravel 9.52.7 to 9.52.8, which also upgraded the deep dep doctrine/inflector which broke some migrations

Notably the laratrust one which contains $table->foreignId( 'permission_id' )->constrained()->cascadeOnDelete()->cascadeOnUpdate(); which before pointed to permissions and now points to permission which is an unexisting table

[driesvints]

@greg0ire I think for now it's best that we revert this PR since so many are affected. Then you can have some time to think about this.