KnpLabs / DoctrineBehaviors

Doctrine2 behavior traits that help handling Blameable, Loggable, Sluggable, SoftDeletable, Uuidable, Timestampable, Translatable, Tree behavior
http://knplabs.com
MIT License
915 stars 300 forks source link

use SymfonyBundle for security instead of core #727

Open tacman opened 1 year ago

tacman commented 1 year ago

This gets rid of the deprecation error. phpunit tests pass.

tacman commented 1 year ago

I thought the way Symfony deprecations worked is that they back-ported the new classes to the supported versions.

What is the proper way to fix this then?

Kocal commented 1 year ago

I thought the way Symfony deprecations worked is that they back-ported the new classes to the supported versions.

Nope it does not work like that, only 6.2 and high versions are affected.

Anyway, even if you requires symfony/security-bundle ^6.2 to get rid of the deprecation for Symfony 6.2+ users, I'm afraid this won't work for people using Symfony 5.4, 6.0 or 6.1 because of the require and the conflict sections of Symfony Security Bundle 6.2.

To me, there are two solutions.

1. Create a breaking changes

We can restrict the usage of symfony/security-bundle ^6.2 and we make this library only compatible with Symfony 6.2+, but this is is a breaking change.

It means that we must a new major release, and I'm not a big fan just for a deprecation.

2. Create a BC-layer

We can add a BC-layer to keep compatibility for Symfony 5.4/6.0/6.1 and 6.2.

First, you must move symfony/security-bundle and symfony/security-core requirements to suggest, as we don't want them in require anymore (to prevent dependencies conflicts, but I may be wrong)

Then, you must write a wrapper around Symfony\Bundle\SecurityBundle\Security and Symfony\Component\Security\Core\Security, like we did in Symfony 5.0 when the Event class has been removed (see the class Behat\Testwork\Event\Event in https://github.com/Behat/Behat/pull/1256).

I don't have tested it, but it should do the job:

<?php

declare(strict_types=1);

namespace Knp\DoctrineBehaviors\Security;

use Symfony\Bundle\SecurityBundle\Security as SecurityBundle;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Security as SecurityCore;

if (class_exists(SecurityBundle::class)) {
    final class Security {
        public function __construct(
            private SecurityBundle $security,
        ) {
        }

        public function getToken(): ?TokenInterface
        {
            return $this->security->getToken();
        }
    }
} elseif (class_exists(SecurityCore::class)) {
    final class Security {
        public function __construct(
            private SecurityCore $security,
        ) {
        }

        public function getToken(): ?TokenInterface
        {
            return $this->security->getToken();
        }
    }
} else {
    throw new \LogicException(sprintf('You cannot use "%s" because either the Symfony Security Bundle or Symfony Security Core is not installed. If you use Symfony 6.2+, try running "composer require symfony/security-bundle", otherwise run "composer require symfony/security-core".', __CLASS__));
}

Finally, in the UserProvider class, change the dependency to the BC-layer \Knp\DoctrineBehaviors\Security\Security


Anyway, it would be nice if the CI can install and run tests on many Symfony and PHP versions, and also with a --prefer-lowest to install deps to the lowest version (which would have failed due to your actual changes)

raziel057 commented 1 year ago

For information I've done it as suggested in my repo https://github.com/raziel057/DoctrineBehaviors/pull/5/commits/1fa7b2cd5d0395de2747ba939cb84d7b8f8cc39b and it's working fine whether the https://github.com/symfony/security-bundle/blob/6.3/Security.php class is present or not.

I can create a PR in this repository but it would be nice if you could first accept the following PR:

Please note that it's not a problem to let the symfony/security-core dependency because it's also required by other packages.

composer why symfony/security-core
symfony/doctrine-bridge  v6.3.2 conflicts symfony/security-core (<6.0)
symfony/framework-bundle v6.3.2 conflicts symfony/security-core (<5.4)
symfony/password-hasher  v6.3.0 conflicts symfony/security-core (<5.4)
symfony/security-bundle  v6.3.3 requires  symfony/security-core (^6.2)
symfony/security-csrf    v6.3.2 requires  symfony/security-core (^5.4|^6.0)
symfony/security-http    v6.3.2 requires  symfony/security-core (^6.3)
tacman commented 1 year ago

It means that we must a new major release, and I'm not a big fan just for a deprecation.

Symfony 7 is just around the corner. How about a major release then, supporting ^6.4 || ^7?