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

feat: add routes useSupportedLocalesOnly property #6073

Merged
merged 5 commits into from
Jun 15, 2022

Conversation

pjsde
Copy link
Contributor

@pjsde pjsde commented Jun 4, 2022

Adds a control flag so that the Router only validates the locales ​​that are defined in App::$supportedLocales, returning an CodeIgniter\Exceptions\PageNotFoundException::forLocaleNotSupported() if the locale introduced is not supported.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@iRedds
Copy link
Collaborator

iRedds commented Jun 4, 2022

Or maybe it would be easier to bind to App::$supportedLocales?
So that after not changing in 10 places.

@pjsde
Copy link
Contributor Author

pjsde commented Jun 4, 2022

@iRedds With this that can be done in the Routes config with $routes->setLocalePattern('('.implode(config('App')->supportedLocales).')'), but also you can do this $routes->setLocalePattern('[a-z]{2}[_-][A-Z]{2}'), that is, you are not limited to the languages ​​​​that are in $App::supportedLocales and we are free to use it in the way that is most useful at any time.

@iRedds
Copy link
Collaborator

iRedds commented Jun 4, 2022

However, the application locale depends on App::$supportedLocales and App::$defaultLocale.
Yes, you can specify any pattern and specify unsupported languages, but if the languages are not supported, then the application will be assigned a default locale.
Therefore, defining a pattern manually is pointless.
Although maybe I misunderstood something.

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Jun 4, 2022
@kenjis kenjis changed the base branch from develop to 4.3 June 4, 2022 21:27
@kenjis kenjis changed the title Refactor router locale pattern feat: router locale pattern Jun 4, 2022
@kenjis
Copy link
Member

kenjis commented Jun 5, 2022

However, the application locale depends on App::$supportedLocales and App::$defaultLocale.
Yes, you can specify any pattern and specify unsupported languages, but if the languages are not supported, then the application will be assigned a default locale.
Therefore, defining a pattern manually is pointless.

I agree. I don't know why the locale pattern needs to override App::$supportedLocales.

@iRedds
Copy link
Collaborator

iRedds commented Jun 5, 2022

@pjsde

  1. If the pattern (x|y) is used, then this is a capture group. For the {locale}/test/(:any) route, the Controller::method/$1 handler will be passed the locale instead of the expected value. That is, for the URL x/test/1, the value of $1 will be x, not 1.

  2. If the pattern x|y is used, then the locale is not captured, but the route {locale}/test/(:any) will match the URLs y/test/1 and x. Which is wrong, and it will also cause locale detection to fail because no match will be found.

These cases are not covered by tests.

@pjsde
Copy link
Contributor Author

pjsde commented Jun 5, 2022

You are right. What I intended to do was to limit the routes to the languages ​​defined in $App::supportedLocales so that it would give an error if the introduced language was not defined, but this can be done through the filters.

@iRedds
Copy link
Collaborator

iRedds commented Jun 5, 2022

I got your idea. But for now I don't know the solution.
Maybe the team will have some thoughts about it.

@kenjis kenjis added the 4.3 label Jun 7, 2022
@kenjis
Copy link
Member

kenjis commented Jun 8, 2022

@kenjis
Copy link
Member

kenjis commented Jun 9, 2022

If the value doesn’t match a valid locale as defined in the App configuration file, the default locale will be used in it’s place.
https://codeigniter4.github.io/CodeIgniter4/outgoing/localization.html#in-routes

If you don't want this, add option to change the behavior?

E.g.

$routes->useDefinedLocalesOnly(true);

@pjsde
Copy link
Contributor Author

pjsde commented Jun 9, 2022

@kenjis I like that solution. It wil solve the problem.

Do you think there should be a config like $App::$useDefinedLocalesOnly = false; to initially define the state of RouteCollection::$useDefinedLocalesOnly or should it be initialized directly in the class?

@kenjis
Copy link
Member

kenjis commented Jun 9, 2022

  1. We are separating the configuration for each class from the Config\App, like Config\Cookie, Config\Security.
  2. Routes.php is not a class.

So, it seems we need to add a property and a setter method for it.

The config in App is $supportedLocales, so the name is $useSupportedLocalesOnly or something is better?

@pjsde pjsde force-pushed the refactor_router_locale_pattern branch from a7317d9 to 21ebcda Compare June 9, 2022 09:44
@pjsde pjsde changed the title feat: router locale pattern feat: add routes useSupportedLocalesOnly property Jun 9, 2022
@InsiteFX
Copy link
Contributor

InsiteFX commented Jun 11, 2022

The problem that I ran into is when designing a DropDown for lanuages with the country flag the language does not match with the country code.

Example: In the CI Translations the code for Japan is ja but using the Country+Region it is ja-JP or en-JPN there is also
a three character code.

If you look at the flags-icons which are svg images they are named different Japan is jp.

I think we need to change the translation files to the correct 2, 4 or 5 codes to match the language or language+region codes.

Just my opinion.

@pjsde pjsde force-pushed the refactor_router_locale_pattern branch from 21ebcda to 8c91c44 Compare June 11, 2022 14:45
@pjsde
Copy link
Contributor Author

pjsde commented Jun 11, 2022

@InsiteFX

The problem that I ran into is when designing a DropDown for lanuages with the country flag the language does not match with the country code.

I think that is a diferent subject from this PR

I think we need to change the translation files to the correct 2, 4 or 5 codes to match the language or language+region codes.

The Language::getLine() already implements what you are proposing

@kenjis
Copy link
Member

kenjis commented Jun 12, 2022

@pjsde Can you update the explanation in #6073 (comment) ?

@pjsde
Copy link
Contributor Author

pjsde commented Jun 12, 2022

@kenjis done.

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

Thank you!

@kenjis kenjis merged commit 5879a3a into codeigniter4:4.3 Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants