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

[RFC] Symfony Translator assertValidLocale() #27853

Closed
bogdanfinn opened this Issue Jul 5, 2018 · 6 comments

Comments

Projects
None yet
5 participants
@bogdanfinn

bogdanfinn commented Jul 5, 2018

Description
When the translation service is created the default locale is injected in this service during creation. After injecting the service into a controller e.g. you can call the setLocale() Method on the injected service and pass null as an argument for the $locale parameter. The assertValidLocal() function does not throw the InvalidArgumentException, because pre_match() in this case returns 1 when passing null as the second argument. In my opinion this is not correct, because null is not a valid locale. For me the Translator should use then the fallback locale but this is a case that can be discussed because when i set the local manually and it is not correct somebody else might do not want the service to use a different locale. I think if null is passed as the second argument the function should throw the Exception.

Example

     * Asserts that the locale is valid, throws an Exception if not.
     *
     * @param string $locale Locale to tests
     *
     * @throws InvalidArgumentException If the locale contains invalid characters
     */
    protected function assertValidLocale($locale)
    {
        if ($locale === null || 1 !== preg_match('/^[a-z0-9@_\\.\\-]*$/i', $locale)) {
            throw new InvalidArgumentException(sprintf('Invalid "%s" locale.', $locale));
        }
    }```

@bogdanfinn bogdanfinn changed the title from RFC Symfony Translator assertValidLocale() to [RFC] Symfony Translator assertValidLocale() Jul 5, 2018

@ismail1432

This comment has been minimized.

Contributor

ismail1432 commented Jul 5, 2018

IMHO If you pass null as an argument an Exception should be thrown, see here

@bogdanfinn

This comment has been minimized.

bogdanfinn commented Jul 5, 2018

Yes, the example is the function how i would write it. The additional null check is not there in the current Symfony version. In the current version null as an argument won't throw an Exception.

The current implementation of the assertValidLocale() function is this:

    /**
     * Asserts that the locale is valid, throws an Exception if not.
     *
     * @param string $locale Locale to tests
     *
     * @throws InvalidArgumentException If the locale contains invalid characters
     */
    protected function assertValidLocale($locale)
    {
        if (1 !== preg_match('/^[a-z0-9@_\\.\\-]*$/i', $locale)) {
            throw new InvalidArgumentException(sprintf('Invalid "%s" locale.', $locale));
        }
    }

See: https://github.com/symfony/translation/blob/master/Translator.php

And if you not set declare(strict_types=1) in this file the function asserts null as a valid locale.
See here: http://sandbox.onlinephpfunctions.com/code/60cf6a343deed659e9f22f3a4102578a3211a3a9

@ismail1432

This comment has been minimized.

Contributor

ismail1432 commented Jul 6, 2018

Yep !
Maybe we can wait for Core Team member point of view

@xabbuh

This comment has been minimized.

Member

xabbuh commented Jul 6, 2018

The docblock states that a string is expected here which null is not. IIRC we generally don't perform such checks.

@stof

This comment has been minimized.

Member

stof commented Jul 6, 2018

indeed, we currently don't make any guarantee about what happens when you violate the contract of the API (and this is what you do by passing null here). note that the behavior for such violation might change even in patch releases (there is really no BC guarantee for them)

@xabbuh

This comment has been minimized.

Member

xabbuh commented Aug 6, 2018

closing as explained

@xabbuh xabbuh closed this Aug 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment