Skip to content
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

use SymfonyBundle for security instead of core #727

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tacman
Copy link

@tacman tacman commented Mar 14, 2023

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

Copy link

@GerB GerB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to cover the deprecation notice, thanks!

Copy link
Contributor

@Kocal Kocal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Security class is only available on Symfony 6.2, this is not the right fix.

@tacman
Copy link
Author

tacman commented May 16, 2023

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
Copy link
Contributor

Kocal commented May 16, 2023

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 Behat/Behat#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
Copy link
Contributor

raziel057 commented Aug 18, 2023

For information I've done it as suggested in my repo raziel057@1fa7b2c 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
Copy link
Author

tacman commented Oct 16, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants