Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Ldap] Implement pagination#29495
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
kevans91 commentedDec 6, 2018
Note that I'm not 100% confident in my assessment of the hack; it works in our environment, though, where we're frequently querying for 6000+ objects against a server that has a 1000-object limit. I will consult with php-ldap folk on that, though, because the advised 'reset' is clearly not enough. |
52d223d toa610a0fComparekevans91 commentedDec 7, 2018
Sorry for the churn- spent some extra time today and got a local test setup going. The backend in the test configuration must be switched to one that actually supports pagination. I've modified the tests to just skip if we get a failure while querying in these tests, though, in case one needs to run them in an environment that doesn't support pagination. PHP 7.1 apparently still has issues with resetting the page bits, even with the hack, according to Travis. I'm not sure how to rectify at this point- PHP 7.1 isn't actively supported as of this month, so trying to get it fixed there simply will not be feasible. I could hide those parts behind PHP_MAJOR_VERSION == 7 && PHP_MINOR_VERSION == 1 and we live with the fact that it simply won't work and likely no one will notice... I'm unsure of the best approach to take. |
Uh oh!
There was an error while loading.Please reload this page.
kevans91 commentedDec 7, 2018
I've createdphp/php-src#3703 to track my progress in addressing the php-ldap bug. |
kevans91 commentedDec 8, 2018
After closer assessment of extldap in PHP-7.1, there is no way that we can properly reset state there enough to make non-paged queries work following paged queries on the same connection for all servers for a kind of silly reason: LDAP_OPT_SERVER_CONTROLS in ldap_get_option is unimplemented, so we can't filter the paged result OID out of the current set of server controls. To add to the hilarity, ldap_set_option won't accept an empty array for LDAP_OPT_SERVER_CONTROLS, so even if we could filter it from the current set- we can't set the option if there are no other server controls left. This will be the case nearly 100% of the time anyways, because ldap_control_paged_result clobbers the server controlset when it's called to only the paged control OID. A fix for this could be adding a fake OID/value/iscritical set. This is an even nastier workaround than I've already implemented, and I'd really prefer to just indicate to users that they should upgrade to a later PHP version if they want to use non-paged queries following paged queries. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
kevans91 commentedDec 29, 2018
Upstream isn't receptive to fixing the broken abstraction, so this possibly will see a small rewrite in the Thursday/Friday timeline to use ldap_search_ext and friends where possible, depending on what's available in PHP 7.1. |
nicolas-grekas commentedJan 25, 2019
rebase needed please |
kevans91 commentedJan 25, 2019
Rebased and fixed new style issues; the small rewrite mentioned before won't happen until 7.1 becomes de-supported. |
| /** | ||
| * Resets pagination on the current connection. | ||
| * | ||
| * @internal |
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.
not needed as the method is private anyway.
| $con =$this->connection->getResource(); | ||
| $searches =$this->search->getResources(); | ||
| $count =0; | ||
| foreach ($searchesas$search) { |
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.
Can we keep the old way of counting without pagination to avoid n requests?
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.
Hmm... sure, but currently without pagination you should only have the single set of results and thus one trip to through the loop and one request. I hadn't done anything special previously because I figured the PHP overhead was fairly minimal.
fabpot commentedMar 31, 2019
Thank you@kevans91. |
This PR was squashed before being merged into the 4.3-dev branch (closes#29495).Discussion----------[Ldap] Implement pagination| Q | A| ------------- | ---| Branch? | master| Bug fix? | yno| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | N/A (cannot test at the moment)| Fixed tickets | N/A| License | MIT| Doc PR | N/AImplement pagination support in the ExtLdap adapter. In a more abstract sense, other adapters should use a query's pageSize option to determine if pagination is being needed. Pagination is required in some environments that frequently query for more results than the remote server is willing to allow.BC break was avoided by having Query->getResource() return the first result, if available.A small hack is included to work around php-ldap failing to reset pagination properly; the LDAP_OPT_SERVER_CONTROLS are sent with every request, whether pagination has been 'reset' by sending a 0-page request or not. This appears to be a php-ldap bug that will need to be addressed there, but we can work-around it for now by doing both: setting the 0-page option *and* unsetting the OID directly. This was resulting in odd results, like queries returning 0 results or returning < server_page_size results for a query that should have returned >= server_page_size.Commits-------b963474 [Ldap] Implement pagination
Implement pagination support in the ExtLdap adapter. In a more abstract sense, other adapters should use a query's pageSize option to determine if pagination is being needed. Pagination is required in some environments that frequently query for more results than the remote server is willing to allow.
BC break was avoided by having Query->getResource() return the first result, if available.
A small hack is included to work around php-ldap failing to reset pagination properly; the LDAP_OPT_SERVER_CONTROLS are sent with every request, whether pagination has been 'reset' by sending a 0-page request or not. This appears to be a php-ldap bug that will need to be addressed there, but we can work-around it for now by doing both: setting the 0-page optionand unsetting the OID directly. This was resulting in odd results, like queries returning 0 results or returning < server_page_size results for a query that should have returned >= server_page_size.