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

Symfony 3: updates to @famoser's fork #25

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

Conversation

sneakyvv
Copy link

@sneakyvv sneakyvv commented May 2, 2017

famoser's fork causes the following error

FatalThrowableError in ShibbolethListener.php line 54:
Type error: Argument 6 passed to KULeuven\ShibbolethBundle\Security\ShibbolethListener::__construct() must be an instance of Symfony\Component\HttpKernel\Log\LoggerInterface, instance of Symfony\Bridge\Monolog\Logger given, called in /var/www/html/var/cache/dev/appDevDebugProjectContainer.php on line 3498

Symfony\Component\HttpKernel\Log\LoggerInterface is deprecated since Symfony 2.2 (http://api.symfony.com/2.8/Symfony/Component/HttpKernel/Log/LoggerInterface.html) and has been removed in Symfony 3.0, causing this error.

It should use Psr\Log\LoggerInterface instead.

Also, Symfony\Component\Security\Core\SecurityContextInterface is declared, but never used.

Since I was not able to create an issue in famoser's fork, I created a separate fork for this.

@sneakyvv sneakyvv mentioned this pull request May 2, 2017
@rmoreas
Copy link
Owner

rmoreas commented Jun 21, 2017

Thanks for the contribution!
Just wondering how are we gonna handle backwards compatibility? Probably there are people still using Symfony 2.x

@sneakyvv
Copy link
Author

I guess you would either need two "master branches", i.e. master-sf3 and master-sf2, or if perhaps tagging/versioning the current state would also work.

PS: I suppose you reacted to a notification about my lastest commit (cc508e7). That should probably not be included in the bundle, as it is a fix for my project specifically. I'm needing the u-number instead of the whole email address as a user name. However, since my fork is a fork of your fork, I didn't manage to require it as a separate branch in my composer.json file, so I pushed it to my master branch.
Perhaps we can solve this issue by introducing some kind of hook which would allow altering the username attribute before using it to fetch a user via the UserProvider?

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.

3 participants