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 https://github.com/doctrine/inflector/issues/221#issuecomment-1594605822

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.

[greg0ire]

On it.

[greg0ire]

Sorry, reopening this since we still need to have a discussion about whether we consider this a breaking change somewhere, and the decision still needs to be clearly documented.

[carlos9108]

same here with laravel. My Nature model gets pluralized to model, which is a problem for my database

[greg0ire]

Folks… this is fixed already, and although I know you mean well, your confirmations don't help. I'm fully aware this is an issue since after reading the very first message. Run composer update, things should go back to normal with the freshly released https://github.com/doctrine/inflector/releases/tag/2.0.8 .

[Tofandel]

@greg0ire Seeing this is the main goal of the library and it's in the doctrine namespace, meaning it's mainly used for determining table names, I would definitely consider anything changing the list of words a breaking change for a major version if we follow semver

Another option would be a minor (but definitely not a patch, as that would remove the possibility to do fixes without behavior change, dep upgrade etc) and people should pin the dependency to 2.0.* instead of ^2.0.0 when they use it to determine something that would break if changed, and keep ^2 if they just use it for something else like spellcheck or display that will not break anything

For the latter, because it requires all library that already use it to change something, so if we go this route, it would be better if the policy takes effect in the next major

[greg0ire]

This will mean potentially many major releases, but I think give how impactful this library is, we shouldn't see it as a blocker. It certainly isn't for doctrine/coding-standard. I disagree with your stance about the minor, the intent behind #216 was to fix things, not to improve them IMO, so I think the choice is really between patch and major. Specifically, "air" is clearly uninflectable.

If I'm wrong and we should go with a minor, then people should still not pin the dependency IMO, they should use ~ instead of ^, so that they still get patches.

[Tofandel]

2.0.* is the same as ~2.0.0 but maybe I used "pin" incorrectly here, sorry

We are on the same page regarding other patches, it's better to have a distinct layer for patches and behavior change, we could consider word change a feature in this regard because it's again the sole purpose of the library

The semver states

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes MINOR version when you add functionality in a backward compatible manner PATCH version when you make backward compatible bug fixes Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

The problem is the definition of API which usually refers to method signatures, return types etc, and not really a string that gets an s or doesn't generally, but in the case of this lib, isn't the list of word to be considered the API?

Otherwise backward compatible manner doesn't have ambiguity here, as we learned the hard way just now, changing the words is not backward compatible in the context of this lib but it can be decided to not follow semver if it makes maintenance much harder

[greg0ire]

2.0.* is the same as ~2.0.0 but maybe I used "pin" incorrectly here, sorry

Oh right sorry, totally missed the star here!

isn't the list of word to be considered the API

Yes, that's the unusual part, that's why we should document our decision, it's not obvious (to us maintainers, and to contributors).

Otherwise backward compatible manner doesn't have ambiguity here

I do think it's ambiguous. Nothing is ever backward compatible, as illustrated by this xkcd:

[greg0ire]

Took a look, the inflector is used in the ORM when:

• generating entities, which is deprecated;
• converting a Doctrine 1 schema which is deprecated as well for obvious reasons;
• reverse-engineering mapping metadata from a database, which is not something people do every day, and certainly not more than once per project;
• resolving magic calls to EntityRepository::{findBy,findOneBy,countBy}* (which we might want to deprecate at some point, since it's hard to analyse statically, I don't even know if it's possible in fact).

It is not used in any of the built-in naming strategies. Singular is used for table names (because entities are singular).

Symfony has no dependency on doctrine/inflector, but the extra package symfony/maker-bundle does. But since it's used once via a command rather than in production all the time, that kind of change is not an issue for its users I guess.

Sylius depends on this library, but I don't know what for.

I guess this explains why we got complaints only from Laravel users so far: to me, it seems to be the framework relying the most heavily on this library right now.

I think a consequence of this is that somebody from the Laravel team should watch this repo and make sure to review all incoming PRs (which are very few).

Since this repo is not very active, I think it wouldn't be unrealistic to bump the major version in cases such as #216.

Next steps:

• create a PR documenting the new BC promise for this repo
• try part of #216 again, on 3.0.x this time.

[derrabus]

All right. Any more lectures on SemVer or your personal interpretation thereof in the context of this library and I will hide your post immediately. That's not getting us anywhere, sorry.

[derrabus]

This library does not and will never cover all pluralization rules of the English language. Not to mention other languages.

The consequence is that it will always be incomplete and new rules are going to be added as soon as someone stumbles across a bad plural or singular form emitted by this library.

And this strategy has always been fine for the use-case this library was built for: one-time code generation.

If you rely on the conversion made by inflector to be stable, I see two options:

• Pin inflector to exactly the release you're using.
• Fork inflector

[driesvints]

Understood @derrabus, thank you for clarifying that. I think personally in that case we'll look into an approach where we'd tackle this in Laravel itself. We'd definitely want the outcome of the generation to stay consistent but I totally understand if that wasn't the intention of this library. I'll try to see how we can move into a different direction.

[mbabker]

Sylius depends on this library, but I don't know what for.

The Sylius\Component\Resource\Metadata\Metadata object uses it for pluralizing metadata names and their core bundle adds a pluralization rule to ensure "taxon" is pluralized to "taxons".