-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
make ILDAPProviderFactory usable when there is no ldap setup #25326
Conversation
There was a problem hiding this 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.
I was also considering not having the |
b717173
to
2700a32
Compare
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! |
0d7d69a
to
3d7d7b2
Compare
Master is Nextcloud 22 now. |
Signed-off-by: Robin Appelman <robin@icewind.nl>
3d7d7b2
to
65b7851
Compare
Rebased |
$factory = new $factoryClass($this); | ||
return new $factoryClass($this); | ||
}); | ||
$this->registerService(ILDAPProvider::class, function (ContainerInterface $c) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Done. |
Currently there is no nice way to inject an
LDAPProvider
into a class that has optional ldap support, by adding a dummyILDAPProviderFactory
one can be injected even if no ldap is setup,ILDAPProviderFactory::isAvailable
can then be used to determine if ldap is available.