Skip to content

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Apr 24, 2023

Summary

PR #34772 broke the fix from PR #35070 because AccessFactory was reusing the same Manager instance for all access instances.

Checklist

It must not reuse the same OCA\User_LDAP\User\Manager instance for
 several Access instances.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added bug 3. to review Waiting for reviews labels Apr 24, 2023
@come-nc come-nc self-assigned this Apr 24, 2023
@come-nc come-nc requested a review from blizzz April 24, 2023 14:07
@come-nc come-nc added this to the Nextcloud 27 milestone Apr 24, 2023
@come-nc
Copy link
Contributor Author

come-nc commented Apr 24, 2023

/backport to stable26

@come-nc come-nc requested review from a team, ArtificialOwl and icewind1991 and removed request for a team April 24, 2023 14:09
@come-nc

This comment was marked as outdated.

@evilham
Copy link
Contributor

evilham commented Apr 26, 2023

Tested this patch, it solves the issue on NC 26

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc requested review from artonge and szaimen April 27, 2023 07:18
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

🐘

$connection,
$this->ldap,
$this->userManager,
Server::get(Manager::class),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment to explain why we do not use DI? It will help to not revert in the future.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 27, 2023
@come-nc come-nc enabled auto-merge April 27, 2023 09:22
@come-nc come-nc disabled auto-merge May 2, 2023 15:10
@come-nc come-nc merged commit c995428 into master May 2, 2023
@come-nc come-nc deleted the fix/user_ldap-fix-multiple-ldap-support branch May 2, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Multiple Ldap doesn't working

6 participants