cleanup: use roles constants

[COil]

Before we had to hard-code roles strings:

use Sensio\Bundle\FrameworkExtraBundle\Configuration\Security;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted;

class PostController extends Controller
{
    /**
     * @IsGranted("ROLE_ADMIN")
     *
     * or use @Security for more flexibility:
     *
     * @Security("is_granted('ROLE_ADMIN') and is_granted('ROLE_FRIENDLY_USER')")
     */
    public function index()
    {
        // ...
    }
}

But with attributes, can use constants. I find this cleaner. I have already used this on several projects and I didn't find drawbacks.

[javiereguiluz]

Could we use an Enum instead of constants? Would Enums work everywhere? Is it worth it or is it overengineering?

[stof]

No we cannot. Roles are strings in Symfony, not an enum object.

[wouterj]

You can use e.g. RoleEnum::USER->value (since PHP 8.1) in attributes and php code, bit IIRC not in Yaml.

[COil]

You can use e.g. RoleEnum::USER->value (since PHP 8.1) in attributes and php code, bit IIRC not in Yaml.

With PHP 8.2 it will be OK, but we can't yet with 8.1:

[COil]

Hello, is it OK for this PR? Or should we close it?

[javiereguiluz]

My opinion:

• 🟢 I like a lot using these constants in PHP files
• 🟡 I'm not entirely sure about injecting these constants in all Twig templates as global constants
• 🔴 I don't like how complicated this looks in the YAML file, especially in the security.yaml file

What do others think about this? Thanks.

[derrabus]

I fully agree with @javiereguiluz' assessment.

🟢 I like a lot using these constants in PHP files

Me too, I wouldn't make those constants members of the User class though. In my projects, I usually have a final class Roles that holds those constants.

🟡 I'm not entirely sure about injecting these constants in all Twig templates as global constants

Me neither and I wouldn't do it tbh. We don't gain much here.

🔴 I don't like how complicated this looks in the YAML file, especially in the security.yaml file

Same. I would revert this as well.

[stof]

Sure you can workaround the bad architecture by using RoleEnum::USER->value everywhere. But I would not recommend doing that in the demo as this would give the impression that using a enum for roles is a good idea, while this is an abuse of enums based on the fact that you need role strings anyway.

[javiereguiluz]

@stof but in this case we're not talking about enums. It's using PHP constants in YAML that looks too complicated to us. E.g. this change:

     role_hierarchy:
-        ROLE_ADMIN: ROLE_USER
+        !php/const App\Entity\User::ROLE_ADMIN: !php/const App\Entity\User::ROLE_USER

But in PHP files these constants look very nice.

[COil]

Me too, I wouldn't make those constants members of the User class though. In my projects, I usually have a final class Roles that holds those constants.

But that looks weird to put this in a separated class. In this case, we should use an enum (but we don't want too here). Having these constants in the User class is the more pragmatic way, IMHO.

🟡 I'm not entirely sure about injecting these constants in all Twig templates as global constants
Me neither and I wouldn't do it tbh. We don't gain much here.

Actually, if we don't want to use a constant here, we could use a twig extension to do the job and make use of the PHP constant. But that's not as simple as using is_granted indeed.

I remember now, that was the opportunity to show the usage of the Twig globals.

🔴 I don't like how complicated this looks in the YAML file, especially in the security.yaml file

I agree, it makes the YAML unreadable. I didn't even know it was possible to do this, so let's revert this. It was a try.

[derrabus]

But that looks weird to put this in a separated class.

🤨 🤷🏻‍♂️

[javiereguiluz]

I like this proposal a lot. Thanks Loïc!

[javiereguiluz]

It's merged now. Thanks!