Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Ldap] Bypass the use ofldap_control_paged_result on PHP >= 7.3#38392

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@lucasaba
Copy link
Contributor

@lucasabalucasaba commentedOct 3, 2020•
edited by nicolas-grekas
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#38352
LicenseMIT
Doc PR

As stated on#38352ldap_control_paged_result andldap_control_paged_result_response have been deprecated since PHP 7.4 and will be removed on PHP 8.0.

With this fix, Query uses serverctrls to handle LDAP results pagination.

Sinceserverctrls where introduced in PHP 7.3 and they are the only way to circumvent the usage ofldap_control_paged_result, I've added a new Query class implementation which usesserverctrls to control pagination.

To do so I've also had to update the LDAP Adapter in order to use the new class if PHP version 7.3 or greater are found

@lucasaba
Copy link
ContributorAuthor

Side note: to test the component I've created an OpenLDAP instance on docker using latest osixia/openldap. TO make test working, I had to addldap_set_option($h, LDAP_OPT_PROTOCOL_VERSION, 3); inLdapTestCase.

Even with this configuration, some test fails with messageFailed asserting that actual size 2 matches expected size 1.. This happens both in my version and in 4.4 version of Symfony.

It would be a good idea to have a docker environment prepared to test those methods....at least in order to have a common environment.

@lucasabalucasaba marked this pull request as ready for reviewOctober 4, 2020 10:13
@nicolas-grekasnicolas-grekas added this to the4.4 milestoneOct 5, 2020
@nicolas-grekasnicolas-grekas changed the titleBypass the use ofldap_control_paged_result if PHP version is 8.0 o…[Ldap] Bypass the use ofldap_control_paged_result on PHP >= 7.3Oct 5, 2020
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Please add types (arguments+return) on methods when possible.

lucasaba reacted with thumbs up emoji
@nicolas-grekas
Copy link
Member

It would be a good idea to have a docker environment prepared to test those methods....at least in order to have a common environment.

I agree. We run such tests in.github/workflows/tests.yml if you'd like to have a look (could be done in a separate PR).

@nicolas-grekas
Copy link
Member

Thank you@lucasaba.

@nicolas-grekasnicolas-grekas merged commit45cad64 intosymfony:4.4Oct 7, 2020
@lucasaba
Copy link
ContributorAuthor

Thanks to you all! Has been a pleasure and a precious lesson!
Now I'll see .github/workflows/tests.yml and try to create the LDAP service.

nicolas-grekas, chalasr, and derrabus reacted with heart emoji

@fabpotfabpot mentioned this pull requestOct 14, 2020
This was referencedOct 28, 2020
Nek- added a commit to Nek-/symfony that referenced this pull requestOct 28, 2020
After PRsymfony#38392 there is a little issue in the Symfony code base thatoccurs only for PHP 7.4 and PHP 8.0.This is related to issuesymfony#38874
Nek- added a commit to Nek-/symfony that referenced this pull requestOct 29, 2020
After PRsymfony#38392 there is a little issue in the Symfony code base thatoccurs only for PHP 7.4 and PHP 8.0.This is related to issuesymfony#38874
Nek- added a commit to Nek-/symfony that referenced this pull requestOct 29, 2020
After PRsymfony#38392 there is a little issue in the Symfony code base thatoccurs only for PHP 7.4 and PHP 8.0.This is related to issuesymfony#38874
Nek- added a commit to Nek-/symfony that referenced this pull requestOct 29, 2020
After PRsymfony#38392 there is a little issue in the Symfony code base thatoccurs only for PHP 7.4 and PHP 8.0.This is related to issuesymfony#38874
@lucasabalucasaba mentioned this pull requestNov 7, 2020
@jderussejderusse mentioned this pull requestNov 7, 2020
fabpot added a commit that referenced this pull requestNov 9, 2020
This PR was merged into the 4.4 branch.Discussion----------[Ldap] Fix pagination| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |#38874| License       | MIT| Doc PR        | N/AThis replaces#38875 to fix a bug introduced by#38392Commits-------4fe0a6f Fix LDAP pagination
chalasr added a commit that referenced this pull requestNov 16, 2020
This PR was merged into the 4.4 branch.Discussion----------Fix critical extension when reseting paged control| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -The issue has been introduced here in#38392 and prevent performing an operation after fetched a paginated result => ldap throws an `Could not XXX: Critical extension is unavailable`At this line:https://github.com/symfony/symfony/pull/38392/files#diff-24b79f3ac1a99938f5acb158a450e38d30c1984a5d333b5b20f2c38a73d10e31L183, the previous code called `ldap_control_paged_result($con, 0);` using the default value (`false`) for the `$critical` argument.The replaced version always use `true`.This PR restore the previous behavior by using `false` when reseting the pagination.Commits-------a2b7476 Fix critical extension when reseting paged control
@jderussejderusse mentioned this pull requestMay 14, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@derrabusderrabusderrabus left review comments

@stofstofstof requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@lucasaba@nicolas-grekas@stof@derrabus@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp