This looks like a new feature to me, I would target master.

This looks like a new feature to me, I would target master.

Changed to master

Changed to master

Thank you, but now it should target 4.4, as master is for Symfony 5 features now ;-)

Thank you, but now it should target 4.4, as master is for Symfony 5 features now ;-)

It seems that documentation needs to be improved https://symfony.com/doc/current/contributing/code/pull_requests.html#choose-the-right-branch

Thank you, but now it should target 4.4, as master is for Symfony 5 features now ;-)

@OskarStark can you please explain how it should be (I'd like to prepare PR):

• bug fixes go to corresponded branch
• new feature go to master branch (5.x)

but why then this PR, should be pointed to 4.4?
also it seems that it contradicts with @fabpot comment

https://symfony.com/doc/current/contributing/code/pull_requests.html#choose-the-right-branch

Currently we are in state where 4.3 is stable, 4.4 will be the next version with new features and master will be the new 5.0 major version. So 4.4 should be the correct branch.

This looks like a new feature to me, I would target master.

The comment by @fabpot was made before the new 4.4 branch was born, correct @fabpot ?

cc @javiereguiluz

New features should indeed target the 4.4 branch. It's not the master branch as we have 2 new versions at the same time: 4.4 and 5.0 (which is special as it happens only once every two years).

Alright I've created symfony/symfony-docs#11741, so this PR can be finally reviewed.

Tests for low are broken, which means that some constraints need to be changed (SecurityBundle depends on the Security component).

@fabpot the only way is to skip SecurityTest::testUserWillBeMarkedAsChangedIfRolesHasChanged for low, because it's a new test which was added in this PR.

Or there is another solution for such case?

You can bump the minimum version for the dependency on SecurityBundle.

and it will pull changes from the current branch, right?

@oleg-andreyev yes :)

@xabbuh thanks for explaining, didn't know that

Just some minor comments, but what I am more concerned about is that this will also log out users if they have been granted new roles. I suspect that this is something that is not expected.

I can understand this concern but theoretically we can add new role which actually limits previous roles and because we can't detect it the easiest way to ensure that ROLE will be taken into account is to logout user and force them to login again.

Ready for review.

Travis failed with:

1) Symfony\Component\Mailer\Tests\TransportTest::testFromDsnMailgun
stream_get_meta_data() expects parameter 1 to be resource, string given

@nicolas-grekas @fabpot @xabbuh @OskarStark please take a look.
Fail build is not related to changes in this PR.

@oleg-andreyev Can you squash your commits please?

@oleg-andreyev Can you squash your commits please?

Done.

Thank you @oleg-andreyev.

@oleg-andreyev We are doing something similar in our User entity:

namespace App\Entity;

use Symfony\Component\Security\Core\User\EquatableInterface;

class User implements EquatableInterface
{
    public function isEqualTo(UserInterface $user)
    {
        $isEqual = count($this->getRoles()) === count($user->getRoles());
        if ($isEqual) {
            foreach ($this->getRoles() as $role) {
                $isEqual = $isEqual && in_array($role, $user->getRoles());
            }
        }

        return $isEqual;
    }
}

Does that mean we can remove User::isEqualTo once we upgrade to Symfony 4.4? Will this feature have the same result?

@XWB yes, results will be the same.

@oleg-andreyev Thank you.