Skip to content

make ILDAPProviderFactory usable when there is no ldap setup #25326

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.

Sign up for GitHub

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

Jump to bottom
Merged
MorrisJobke merged 1 commit into from
Mar 31, 2021
Merged

make ILDAPProviderFactory usable when there is no ldap setup #25326

MorrisJobke merged 1 commit into from
Mar 31, 2021

Conversation

@icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Jan 25, 2021

Currently there is no nice way to inject an LDAPProvider into a class that has optional ldap support, by adding a dummy ILDAPProviderFactory one can be injected even if no ldap is setup, ILDAPProviderFactory::isAvailable can then be used to determine if ldap is available.

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Jan 25, 2021
@icewind1991 icewind1991 added this to the Nextcloud 21 milestone Jan 25, 2021
blizzz
blizzz approved these changes Jan 25, 2021
View reviewed changes
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

Good one. API change should be reflected in critical changes, but I don't expect (and am not aware) that there is any custom implementation out there.

@icewind1991
Copy link
Member Author

icewind1991 commented Jan 25, 2021

I was also considering not having the isAvailable method and instead throwing an exception, that would remove the api break but using exceptions for expected behaviour isn't great.

@nextcloud nextcloud deleted a comment from faily-bot bot Jan 25, 2021
@blizzz
Copy link
Member

blizzz commented Jan 25, 2021

I was also considering not having the isAvailable method and instead throwing an exception, that would remove the api break but using exceptions for expected behaviour isn't great.

No, I agree with you. That was not ideally implemented, and my memory tricked me into thinking: it's a factory, that would not throw Exceptions unnecessarily. Thanks for taking care!

@icewind1991 icewind1991 force-pushed the ldap-factory-no-ldap branch 2 times, most recently from 0d7d69a to 3d7d7b2 Compare January 26, 2021 18:04
@nextcloud nextcloud deleted a comment from faily-bot bot Jan 26, 2021
This was referenced Jan 27, 2021
Merged
Merged
@rullzer rullzer modified the milestones: Nextcloud 21, Nextcloud 22 Feb 2, 2021
@rullzer
Copy link
Member

rullzer commented Feb 2, 2021

Master is Nextcloud 22 now.
If this should go into 21 it should be backported.

@icewind1991 icewind1991 force-pushed the ldap-factory-no-ldap branch from 3d7d7b2 to 65b7851 Compare March 31, 2021 13:24
@icewind1991
Copy link
Member Author

icewind1991 commented Mar 31, 2021

Rebased

@icewind1991 icewind1991 mentioned this pull request Mar 31, 2021
Merged
1 task
MorrisJobke
MorrisJobke reviewed Mar 31, 2021
View reviewed changes
lib/private/Server.php
$factory = new $factoryClass($this);
return new $factoryClass($this);
});
$this->registerService(ILDAPProvider::class, function (ContainerInterface $c) {
Copy link
Member

@MorrisJobke MorrisJobke Mar 31, 2021

Choose a reason for hiding this comment

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

I would not allow this. That leads to a direct exception if LDAP is not enabled. I would only allow the way via the factory.

Copy link
Member Author

@icewind1991 icewind1991 Mar 31, 2021

Choose a reason for hiding this comment

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

This exists for BC reasons, mirroring the previous behavior when apps tried to query LDAPProvider

Copy link
Member

@MorrisJobke MorrisJobke Mar 31, 2021

Choose a reason for hiding this comment

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

Got it. I didn't noticed it because of the diff 🙈

MorrisJobke
MorrisJobke requested changes Mar 31, 2021
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

See inline.

MorrisJobke
MorrisJobke approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

👍

@MorrisJobke MorrisJobke merged commit 6a81477 into master Mar 31, 2021
@MorrisJobke MorrisJobke deleted the ldap-factory-no-ldap branch March 31, 2021 19:23
@icewind1991 icewind1991 mentioned this pull request Apr 1, 2021
Merged
@MorrisJobke MorrisJobke mentioned this pull request Apr 1, 2021
Closed
@MorrisJobke
Copy link
Member

MorrisJobke commented Apr 1, 2021

Good one. API change should be reflected in critical changes, but I don't expect (and am not aware) that there is any custom implementation out there.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@blizzz blizzz blizzz approved these changes

@rullzer rullzer Awaiting requested review from rullzer

Assignees

No one assigned

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 22

Development

Successfully merging this pull request may close these issues.

5 participants

@icewind1991 @blizzz @rullzer @MorrisJobke