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

[9.x] Configurable pluralizer language and uncountables #41941

Merged
merged 4 commits into from
Apr 18, 2022

Conversation

cesarep
Copy link
Contributor

@cesarep cesarep commented Apr 12, 2022

Hello!

As said previously on #41891, Pluralizer already handles all the uncountable words, so maintaining a list of those here is really redundant, the only words not listed there were already merged doctrine/inflector#194, with the exception of recommended and related for not being nouns, so i added the recommendation on the comments to request new nouns there, and keep this array for edge cases only. And while they don't release the 2.0.5 version with this merge, cattle and kin also stayed, avoiding any breaking changes.

As for the language configuration, i finally understood the errors on the last PR!
The tests on tests/Database use the Pluralizer as it's an essential part of Eloquent, however, their testcases (and the pluralizer test too) extend PHPUnit\Framework\TestCase instead of Orchestra\Testbench\TestCase, not bootstrapping the application, so it's useless to setup a getEnvironmentSetUp($app) function there. All the tests on features that depend on the Pluralizer and don't bootstrap the app will always throw an error saying the config class doesn't exist.

I used a simple $app->has('config') to check if the instance has this class, to get the config, otherwise it returns the default english version, acting as a fallback if the pluralizer is used outside Laravel, bypassing that error. A more elegant way to check if an Instance exists is most welcome of course.

I'm being insistent on this because I really believe this feature will be very useful for a lot of non-english speaking developers making apps on their languages. Me, personally, I'm using a modified version of Laravel with this.

It might be nice to also add new Orchestra's testcases for the Pluralizer with the other languages, i could contribute with the portuguese test. Inside tests/Integration/Support maybe?

I set the config path to 'app.pluralizer.language', grouping all the pluralizer related configs in one array in case someone finds a way to further extend the pluralizer with custom rules or custom words that might be set from the configs files.

@driesvints
Copy link
Member

@cesarep remember that draft PR's aren't reviewed.

@cesarep
Copy link
Contributor Author

cesarep commented Apr 12, 2022

I know!
I'm making a test file for portuguese to include with this

@crynobone
Copy link
Member

This add a new dependency to illuminate/container

@cesarep
Copy link
Contributor Author

cesarep commented Apr 13, 2022

That's weird when the test is run here it doesn't seem to use the portuguese ruleset
If i change the phpunit configs to run just the tests/Integration/Support folder where the test is located it runs just fine
image
When i do the full tests it seems to ignore that, and it gives the sames mistakes as here.
I'm thinking it has something to do with the static nature of the Inflector Instance, as it's used before this test when the english default is set, it remains the same after. Is there a way to force a reset on that instance?

Or we might as well go without a test. I was looking at the Inflector rules for portuguese anyway and, while it does really helps with probably 90% of the common words, there are still some inconsistencies that i'm reviewing doctrine/inflector#195

@cesarep cesarep marked this pull request as ready for review April 13, 2022 17:56
@cesarep
Copy link
Contributor Author

cesarep commented Apr 13, 2022

In case someone knows a way to workaround this static instance issue, we should reset the instance to the desired languages when the test begins (ideally there should be a test for each available language), and back to english once it finishes, so it doesn't mess up the rest of the tests after it.
But probably is best to leave the language test out for now anyway, is there a more elegant way to remove the last commit of the PR other than a force push?

@taylorotwell taylorotwell merged commit d81043e into laravel:9.x Apr 18, 2022
@taylorotwell
Copy link
Member

taylorotwell commented Apr 18, 2022

Removed container / config dependency. Added Pluralizer::useLanguage(string) method.

@cesarep
Copy link
Contributor Author

cesarep commented Apr 18, 2022

Oh great!!

How should this be setup on our apps?
I'm assuming on the boot function inside AppServiceProvider as we do for custom resourceVerbs for the routes?

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.

None yet

4 participants