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] Fixed issue with legacy client initialisation #18944

Merged
merged 1 commit into from Jun 8, 2016

Conversation

Projects
None yet
6 participants
@csarrazi
Copy link
Contributor

commented Jun 1, 2016

Q A
Branch? 3.1
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #18725 (comment)
License MIT
Doc PR

Thanks @uvups for noticing this.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

Any way to add some tests?

@csarrazi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2016

Sure! I'll try to find some time for this tomorrow

@uvups

This comment has been minimized.

Copy link

commented Jun 2, 2016

@csarrazi This fix worked for me. Thanks

@@ -24,13 +24,22 @@
public function __construct($host = null, $port = 389, $version = 3, $useSsl = false, $useStartTls = false, $optReferrals = false, LdapInterface $ldap = null)
{
if ((bool) $useSsl) {

This comment has been minimized.

Copy link
@stof

stof Jun 2, 2016

Member

no need to cast explicitly here

This comment has been minimized.

Copy link
@csarrazi

csarrazi Jun 2, 2016

Author Contributor

True. However this was kept to ensure the previous behaviour.

I can remove the cast, if needed.

@stof

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

btw, this class is missing its deprecation warning

@csarrazi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

You mean in the constructor, right?

@csarrazi csarrazi force-pushed the csarrazi:fix/ldap-legacy-compatibility branch from 1cd51eb to 40f42e6 Jun 7, 2016

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

@csarrazi stof means after the namespace declaration. See grep '^@trigger' src/ -r (on 2.8)

@csarrazi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2016

Ok. I'll check this tonight! Thanks for the info @nicolas-grekas!

@csarrazi csarrazi force-pushed the csarrazi:fix/ldap-legacy-compatibility branch from 40f42e6 to f655897 Jun 8, 2016

@csarrazi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2016

@@ -11,6 +11,8 @@
namespace Symfony\Component\Ldap;
@trigger_error('The '.__NAMESPACE__.'\LdapClient class is deprecated since version 3.1 and will be removed in 4.0. Use directly the Ldap class instead.', E_USER_DEPRECATED);

This comment has been minimized.

Copy link
@fabpot

fabpot Jun 8, 2016

Member

Use the Ldap class directly instead.

This comment has been minimized.

Copy link
@csarrazi

csarrazi Jun 8, 2016

Author Contributor

Done

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

👍
Just wondering: do the added tests make sense on the Ldap class also? Or will it be OK to just remove them when doing 4.0?

@csarrazi csarrazi force-pushed the csarrazi:fix/ldap-legacy-compatibility branch from f655897 to 6804efe Jun 8, 2016

@csarrazi

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2016

It will be okay to remove them for 4.0. The LdapClientTest tests only happen to test the BC layer:

  • whether the Ldap class is properly encapsulated or not in the LdapClient class;
  • the BC-compatible find() method;
  • the config change between the two classes.
@fabpot

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

Thank you @csarrazi.

@fabpot fabpot merged commit 6804efe into symfony:3.1 Jun 8, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jun 8, 2016

bug #18944 [Ldap] Fixed issue with legacy client initialisation (csar…
…razi)

This PR was merged into the 3.1 branch.

Discussion
----------

[Ldap] Fixed issue with legacy client initialisation

| Q             | A
| ------------- | ---
| Branch?       | 3.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #18725 (comment)
| License       | MIT
| Doc PR        |

Thanks @uvups for noticing this.

Commits
-------

6804efe Fixed issue with legacy client initialization

@csarrazi csarrazi deleted the csarrazi:fix/ldap-legacy-compatibility branch Jun 8, 2016

@fabpot fabpot referenced this pull request Jun 15, 2016

Merged

Release v3.1.1 #19055

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