NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Ldap] Removed deprecated code#41363

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:6.0frommalteschlueter:6.0-ldap-deprecated

May 23, 2021

Merged

[Ldap] Removed deprecated code#41363

nicolas-grekas merged 1 commit intosymfony:6.0frommalteschlueter:6.0-ldap-deprecated

May 23, 2021

Conversation

Copy link

Contributor

malteschlueter commentedMay 21, 2021•
edited by derrabus
Loading

Q	A
Branch?	6.0
Bug fix?	no
New feature?	no
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

This remove deprecated code from Ldap.

Do I have to update the upgrade log for the two method deletions or is it unnecessary because it's mentioned in the security?

malteschlueter requested review fromchalasr andwouterj ascode owners

May 21, 2021 12:38

carsonbot added Status: Needs Review Deprecation Ldap labels

May 21, 2021

carsonbot added this to the6.0 milestone

May 21, 2021

Copy link

Member

derrabus commentedMay 21, 2021

Can you have a look at the Psalm error?

derrabus reviewed

May 21, 2021

View reviewed changes

src/Symfony/Component/Ldap/Security/LdapUser.php OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

malteschlueter commentedMay 21, 2021

Can you have a look at the Psalm error?

I will revert the deletion of the methods. Probably it's better to delete it if somebody starts to clean up theUserInterface

malteschlueter commented

May 21, 2021

View reviewed changes

src/Symfony/Component/Ldap/Security/CheckLdapCredentialsListener.php


		$user =$passport->getUser();
		if (!$userinstanceof PasswordAuthenticatedUserInterface) {
		trigger_deprecation('symfony/ldap','5.3','Not implementing the "%s" interface in class "%s" while using password-based authenticators is deprecated.', PasswordAuthenticatedUserInterface::class,get_debug_type($user));

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Should this now also a exception?

Copy link

Member

Choose a reason for hiding this comment

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

IMHO it does not need to, we can just remove the deprecation here.
Reason for this notice to be here is that in order to removegetPassword() from the generic UserInterface, we needed to trigger deprecation notices from as much places as possible to make sure developers get it. Here we don't use anything fromPasswordAuthenticatedUserInterface so nothing to do.

Copy link

Member

derrabus commentedMay 21, 2021

I will revert the deletion of the methods. Probably it's better to delete it if somebody starts to clean up theUserInterface

I think, deletinggetUsername() is fine. The Psalm error should be fixed by#41367.

Copy link

Member

derrabus commentedMay 21, 2021

Please rebase, I've merged my two PRs up.

[Ldap] Removed deprecated code

16bb7c4

derrabus approved these changes

May 21, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

May 21, 2021

chalasr approved these changes

May 21, 2021

View reviewed changes

Copy link

Member

wouterj commentedMay 21, 2021•
edited
Loading

Thanks for all the PRs!

I'm not sure if you were planning to, but I would propose to leave the security packages (other than this one) as-is for now. There is quite some deprecated code, as well as undeprecated but redundant code if we remove deprecation features. So we probably have to remove them per feature instead of per package. Let's make a plan when 5.3 is stable :)