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

[Ldap] Add LDAP error codes to exceptions #28677 #33339

Open
wants to merge 3 commits into
base: 4.4
from

Conversation

@dontub
Copy link

commented Aug 26, 2019

This PR ensures that an LdapException is only used when an LDAP
operations fails. In cases an LdapException was used for non LDAP
operations it is replaced by an appropriate exception.

Q A
Bug fix? no
New feature? yes
BC breaks? LdapException requires error code now. (I wouldn't expect it to be relevant for any component user.)
Deprecations? no
Tests pass? yes
Fixed tickets #28677
License MIT

This PR ensures that an LdapException is only used when an LDAP operations fails. The constructor of LdapException enforces that an error code is specified. In cases an LdapException was used for non LDAP operations it is replaced by an appropriate exception.

Additionally silencing is added to ldap functions where missing.

@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

I like your work but to me it’s a BC break not about the error code (which could be handled and deprecated first) but someone who is catching an LdapException now got a different one and the try catch doesn’t work as expected anymore...

This could be reworked b extending LdapException by the newly introduced exceptions which would be BC.

@dontub

This comment has been minimized.

Copy link
Author

commented Aug 27, 2019

This could be reworked b extending LdapException by the newly introduced exceptions which would be BC.

I first thought about it. Though if any of the new exceptions is thrown, the using software cannot work correctly nowadays (and I wanted to avoid extra work to fix the class hierarchy in Sf 5). Nevertheless, you are right, to completely keep the same behavior they should extend LdapException. Do you think this is necessary for DomainException as well? This one is thrown here:

throw new DomainException(sprintf('Could not search in scope "%s".', $this->options['scope']));

Though that we have a valid scope is already ensured by the OptionsResolver:
$resolver->setAllowedValues('scope', [static::SCOPE_BASE, static::SCOPE_ONE, static::SCOPE_SUB]);

@dontub

This comment has been minimized.

Copy link
Author

commented Aug 30, 2019

BC is now ensured (apart from DomainException, see comment above). That the checks fail doesn't seem to be my fault.

Dominic Tubach added 3 commits Aug 26, 2019
This PR ensures that an LdapException is only used when an LDAP
operations fails. In cases an LdapException was used for non LDAP
operations it is replaced by an appropriate exception.
Style fixes
Dominic Tubach
@dontub dontub force-pushed the dontub:ldap_exceptions-ticket_28677 branch from c2f96d8 to 2dede53 Oct 9, 2019
@dontub

This comment has been minimized.

Copy link
Author

commented Oct 9, 2019

Let me know if anything is left to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.