[Ldap] Implement pagination #29495
[Ldap] Implement pagination #29495
Conversation
|
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 to
a610a0f
Compare
|
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. |
|
I've created php/php-src#3703 to track my progress in addressing the php-ldap bug. |
a610a0f to
ed3dc5c
Compare
|
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. |
ed3dc5c to
9d77113
Compare
|
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. |
|
rebase needed please |
9d77113 to
d798080
Compare
|
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.
not needed as the method is private anyway.
| $con = $this->connection->getResource(); | ||
| $searches = $this->search->getResources(); | ||
| $count = 0; | ||
| foreach ($searches as $search) { |
There was a problem hiding this comment.
Can we keep the old way of counting without pagination to avoid n requests?
There was a problem hiding this comment.
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.
|
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/A 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 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 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.