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

default_locale not passed to UrlGenerator #30996

Closed
HypeMC opened this issue Apr 8, 2019 · 8 comments
Closed

default_locale not passed to UrlGenerator #30996

HypeMC opened this issue Apr 8, 2019 · 8 comments

Comments

@HypeMC
Copy link
Contributor

HypeMC commented Apr 8, 2019

Symfony version(s) affected: >=4.1

Description
Symfony 4.1 introduced Internationalized routing.
After defining a route with multiple locale I tried to generate a url with a command and got the following error:

Unable to generate a URL for the named route "locale" as such route does not exist.

The route:

    /**
     * @Route({
     *    "en": "/locale",
     *    "hr": "/lokalno"
     * }, name="locale")
     */

The code that generates the url:

$this->router->generate('locale');

After some playing around I realized that I need to explicitly set the locale in order for the url to be generated, which was strange since I had the framework.default_locale set to hr.

// This works
$this->router->generate('locale.hr');

I did a little digging and noticed that the UrlGenerator class has a $defaultLocale parameter and the following code:

$locale = $parameters['_locale']
    ?? $this->context->getParameter('_locale')
    ?: $this->defaultLocale;

The problem is that the $defaultLocale parameter is never set when the class is instantiated. I would expect the framework.default_locale parameter to be passed here.

How to reproduce

# framework.yaml
framework:
    default_locale: hr
// Controller
class LocaleController extends AbstractController
{
    /**
     * @Route({
     *    "en": "/locale",
     *    "hr": "/lokalno"
     * }, name="locale")
     */
    public function index()
    {
        // ...
    }
}
// Command
class LocaleCommand extends Command
{
    protected static $defaultName = 'app:locale';

    // ...

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $output->writeln($this->router->generate('locale'));
    }
}

Possible Solution

A possible solution would be to override the way the UrlGenerator class is instantiated in the FrameworkBundle Router class and pass the framework.default_locale as the $defaultLocale parameter.

@fancyweb
Copy link
Contributor

fancyweb commented Apr 8, 2019

IIRC, it is possible to have an i18n route "my_route" (so its names are technically my_route.en, my_route.hr, etc.) and an unrelated route with the name "my_route".

Passing the default locale to the UrlGenerator would be a BC break because it would generate another route than currently with the default value.

I think we need to deprecate having a route with the same name than an i18n route so we can actually do something about this in Symfony 5.0.

Kind of related to : #30010

@HypeMC
Copy link
Contributor Author

HypeMC commented Apr 8, 2019

Not sure it would be a BC break since the problem is that there's inconsistent behavior between the web and cli contexts. The route exists when generated in a web request cause it gets the locale from the request context ($this->context->getParameter('_locale')):

class LocaleController extends AbstractController
{
    /**
     * @Route({
     *    "en": "/locale",
     *    "hr": "/lokalno"
     * }, name="locale")
     */
    public function index()
    {
        // This works
        return new Response($this->generateUrl('locale'));
    }
}

@X-Coder264
Copy link
Contributor

X-Coder264 commented Apr 8, 2019

Currently the same route generation call behaves differently depending on the context it's being run in (WEB or CLI). In a WEB context the above route would be generated just fine, while in CLI it'd throw an exception. The system does not behave consistently and I don't see how this can be treated as anything but a bug.

Another solution which would avoid this "breaking change" and allow us to make the behavior consistent no matter the context is adding another parameter (let's call it router.request_context.defaut_locale for example) which would work the same as all other similar parameters as described in https://symfony.com/doc/current/console/request_context.html

Since that parameter doesn't exist yet it would always default to null unless explicitly set so the current behavior remains the same while it allows developers to override/customize it.

@stof
Copy link
Member

stof commented Apr 8, 2019

Adding this new parameter looks good to me.

@HypeMC
Copy link
Contributor Author

HypeMC commented Apr 8, 2019

@stof Hm, but would we then have two different parameters/configuration options for the same thing, framework.default_locale & router.request_context.defaut_locale?

@stof
Copy link
Member

stof commented Apr 8, 2019

We don't have framework.default_locale. We have framework.translator.default_locale, which will not exist at all if you don't enable the translator. So we cannot make it a hard dependency.

@HypeMC
Copy link
Contributor Author

HypeMC commented Apr 8, 2019

@stof Not sure what you mean, I was referring to the FrameworkBundle configuration, which is set as kernel.default_locale.

The default locale is used if no _locale routing parameter has been set.

@stof
Copy link
Member

stof commented Apr 8, 2019

hmm indeed. I confused things. Using the kernel.default_locale would indeed make sense then.

@fabpot fabpot closed this as completed Apr 22, 2019
fabpot added a commit that referenced this issue Apr 22, 2019
…264)

This PR was squashed before being merged into the 4.2 branch (closes #31023).

Discussion
----------

[Routing] Fix route URL generation in CLI context

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30996
| License       | MIT
| Doc PR        | -

This fixes #30996 and makes URL generation in the CLI context behave the same as it does in the web context where the `LocaleListener` sets the default locale (to the router context).

The Travis CI failure is related to the fact that the constraint for `symfony/routing` should be bumped to `^4.2.6` in the composer.json of the FrameworkBundle (when it gets tagged).

Commits
-------

4a1ad4a [Routing] Fix route URL generation in CLI context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants