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] Allow for an integer $type on ControllerTrait::addFlash() and FlashBagInterface::add() #27520

Closed
Aerendir opened this Issue Jun 6, 2018 · 27 comments

Comments

Projects
None yet
7 participants
@Aerendir

Aerendir commented Jun 6, 2018

Currently both ControllerTrait::addFlash() and FlashBagInterface::add() have a string type hint.

This is not useful to set messages.

I found myself using Monolog Logger constants to set the type of messages:

use Monolog\Logger;

$this->addFlash(Logger::ERROR, 'An error happened');

But using declare(strict_types=1) cause this to be considered an error (correctly).

I suggest remove the method type hint and add a check inside the ControllerTrait::addFlash() method:

    /**
     * Adds a flash message to the current session for type.
     *
     * @throws \LogicException
     *
     * @final
     */
    protected function addFlash($type, string $message)
    {
+        if (!is_string($type) && !is_int($type)) {
+            throw new \InvalidArgumentException('Only strings and ints are accepted');
+        }
        
        if (!$this->container->has('session')) {
            throw new \LogicException('You can not use the addFlash method if sessions are disabled. Enable them in "config/packages/framework.yaml".');
        }

        $this->container->get('session')->getFlashBag()->add($type, $message);
    }

According to this, also the FlashBagInterface::add() DocComment has to be updated:

    /**
     * Adds a flash message for type.
     *
-     * @param string $type
+     * @param string|int $type
     * @param string $message
     */
    public function add($type, $message);

The classes that implement FlashBagInterface don't have any method type hint and inherit the DocBlock from FlashBagInterface, so no changes are required.

CONCRETE PROBLEM

This type hint causes concrete problems.

In fact I could bypass the type hinting forcing using the typecasting of the constants provided by Logger:

use Monolog\Logger;

- $this->addFlash(Logger::ERROR, 'An error happened');
+ $this->addFlash((string) Logger::ERROR, 'An error happened');

But this cause problems in Twig:

<div class="Notifications">
    {% for notification in notifications %}
        {# INFO #}
        {% if 200 === notification.level %}
            {% set level = 'info' %}
...
@Tobion

This comment has been minimized.

Member

Tobion commented Jun 6, 2018

Changing the interface would be a bc break. See http://symfony.com/doc/current/contributing/code/bc.html

To me it does not seem worth to create a bc break for this.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 6, 2018

@Tobion , it is not a BC as the string type hint remains: all the current code will continue to work as expected and as usual.

More, the change is not code related, but only affects the DocBlock: see the code I posted.

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jun 6, 2018

It is typehinted to string since 4.0, so typehint can't really be taken away now.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 6, 2018

Ok, the problem is this: we have a helper method in the ControllerTrait that forces to use only strings and the real method FlashBagInterface::add() that doesn't: this is a contradiction.

Considering that the $type can also be an integer (as demonstrated by the use of Logger), the wrong method is the one in the ControllerTrait: this forces uselessly to use strings.

The fact that it was type hinted in the 4.0 version is irrelevant: if there is an error, there is an error, and it is not relevant when the error was introduced: it has to be removed.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 6, 2018

The method is final so it's ok changing it btw.

@Tobion

This comment has been minimized.

Member

Tobion commented Jun 6, 2018

It's a bc break because implementations of FlashBagInterface::add() might need to support int now which they didn't need before.

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jun 6, 2018

it can be just casted to string before calling add()

@Aerendir

This comment has been minimized.

Aerendir commented Jun 6, 2018

@Tobion , forgive me if I insist: the BC policy involves only the methods that are directly used by the end developer.

It doesn't involve the internals.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 6, 2018

@ostrolucky , yes, for sure: but this is "dirty".

The real point I miss is why the helper has to introduce a restriction that the real method FlashBagInterface::add() hasn't.

More, the type casting forces me to use == instead of === (as I compare against an int, the one provided by the Logger's constants) and this causes a lot of warnings in code analysis... It is a mess...

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Jun 6, 2018

my comment was directed to @Tobion

@Aerendir

This comment has been minimized.

Aerendir commented Jun 6, 2018

@Tobion , more about internal implementation: they are simple arrays: the $type is used only as keys, and array keys can be strings or ints.

These are the only two concrete implementations.

Anyway, I repeat: $type will continue to accept strings: it only will accept also ints.

@iltar

This comment has been minimized.

Contributor

iltar commented Jun 6, 2018

It's pretty common to put objects in flash messages, like a data container for a delayed translation. I've written this code before (and I know it breaks the docblock), but it's a very clean solution. I would prefer that the interface would accept "mixed" instead.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 6, 2018

@iltar , are you speaking about the second param, $message?

@iltar

This comment has been minimized.

Contributor

iltar commented Jun 6, 2018

@Aerendir correct

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 9, 2018

Rereading this, I understand that you're using a number as the name of a flash message?
I think it's correct to fail in strict mode. A number is a very loose name for a message, imagine someone else uses ->set(ANOTHER_CONST, ...) but ANOTHER_CONST also equals Logger::ERROR. Using a number, this is much more likely to collide.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 9, 2018

@nicolas-grekas , I think the messages should be standardized in the team: a scenario like the one you are describing is caused by a poor development habit... The same thing may happen also using strings with the exact same probability...

Anyway, as I already told, there is anyway an inconsistency: one method put the restriction (the one in the trait) while another doesn't (the one in the Session): there are only two alternatives: both of them put restrict to strings or both of them don't.

But having one method that restricts and another that doesn't for me is an inconsistency.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 9, 2018

Anyway, just as a side note, I switched from Monolog\Logger to Psr\LogLevels: this last one uses strings instead of numbers and so the problem is not relevant to me anymore.

But remains the inconsistency for which a clear decision should be taken, IMHO.

@jvasseur

This comment has been minimized.

Contributor

jvasseur commented Jun 9, 2018

There is no inconsistency, the FlashBagInterface states that $type must be a string (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Flash/FlashBagInterface.php#L26) passing anything else means you don't respect the interface contract thus fall into undefined behavior (we event don't guaranteed the BC promise when you don't respect the contract meaning this could break in a patch version).

And changing it would be a BC break because it would means other implementations of FlashBagInterface would have to support receiving an int where they only had to deal with strings before.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 9, 2018

As "inconsistency" I meant there is no type hinting in the concrete implementations... While there is for the helper method of ContainerTrait.

You are right: the interface hints for strings only.

But the concrete implementations don't type hint for them: this is the "inconsistency".

Without static code analysis, someone may pass an int to the SessionBag anyway (and this was exactly what I did until now: sincerely: I don't read the comments of the classes I use. Now I will as I'm using static code analysis).

@jvasseur

This comment has been minimized.

Contributor

jvasseur commented Jun 9, 2018

The lack of type hinting comes from the fact that adding them now would break extending classes for people using PHP < 7.2 so we can't add them now.

And the concrete implementations does hint for strings, they are using {@inheritdoc} meaning they inherit the phpdoc from their parent including the type hints.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 9, 2018

And as a side note: I don't understand what do you mean by "BC".

If until now you had to deal with only strings, nothing changes if you deal also with ints: also if you enable the strict_mode, the current code will continue to work as, until now, you only passed strings.

Different discourse is if you make possible to pass also, for example, null.

But in this specific case I don't see any BC.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 9, 2018

The BC is for other implementations, extending these interfaces/classes: adding the type would break them. What you describe as inconsistent will be fixable with Symfony 5, when PHP will be bumped to 7.2 or higher. For now, we can't do anything I believe.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 9, 2018

Ok @nicolas-grekas , then this issue can be closed if reputed not relevant.

I solved my specific use case, so it is all about Symfony: if you think this is something should be fixed or not...

Or if the type hinting has to be added to the SessionBag class too, or not...

I made you aware that there are methods type hinted and methods that aren't: the choice is yours.

Just for reference:

FlashBagInterface (not type hinted):

    /**
     * Adds a flash message for type.
     *
     * @param string $type
     * @param string $message
     */
    public function add($type, $message);

FlashBag (not type hinted)

    /**
     * {@inheritdoc}
     */
    public function add($type, $message)
    {
        $this->flashes[$type][] = $message;
    }

AutoExpireFlashBag (not type hinted)

    /**
     * {@inheritdoc}
     */
    public function add($type, $message)
    {
        $this->flashes[$type][] = $message;
    }

ControllerTrait (type hinted)

    /**
     * Adds a flash message to the current session for type.
     *
     * @throws \LogicException
     *
     * @final
     */
    protected function addFlash(string $type, string $message)
    {
        if (!$this->container->has('session')) {
            throw new \LogicException('You can not use the addFlash method if sessions are disabled. Enable them in "config/packages/framework.yaml".');
        }
        $this->container->get('session')->getFlashBag()->add($type, $message);
    }
@jvasseur

This comment has been minimized.

Contributor

jvasseur commented Jun 9, 2018

The difference comes from the fact that the addFlash method in the ControllerTrait is marked as final. That allowed us to add the type hint in Symfony 4.0 since we don't have to deal with inheritance. Since the other methods aren't final we can't add the type hint until we require PHP >= 7.2.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 9, 2018

@jvasseur , excuse me again if I insist again: I'm only trying to understand as much as I can...

The composer.json of both symfony/http-fundation and of symfony/framework-bundle require "php": "^7.1.3".

And from what I read, the type declaration (alias type hinting) was added in PHP 7.0.

So, what is the relation between the PHP version and the difference highlighted?

@jvasseur

This comment has been minimized.

Contributor

jvasseur commented Jun 9, 2018

With PHP < 7.2 when you add type declaration to a method argument it means all extending classes have to use the same type declaration, with PHP 7.2 the extending class can remove the type declaration (https://wiki.php.net/rfc/parameter-no-type-variance, https://3v4l.org/f26BQ).

This means you can add type declarations to classes/interfaces without breaking extending classes.

@Aerendir

This comment has been minimized.

Aerendir commented Jun 9, 2018

Yes, it is true! I forgot this particular! You are right, I’m wrong.

Thank you for your immense patience :)

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