-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Feature: multiPageSearch funcation added for Pagination while searchi… #9
Conversation
@heiglandreas |
@ArchTaqi Please have a look on Travis there are some problems listed: https://travis-ci.com/github/laminas/laminas-ldap/jobs/340791227#L836 |
} while ($cookie); | ||
ErrorHandler::stop(); | ||
|
||
if (count($results) == 0) { |
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 use truthy check:
if (count($results) == 0) { | |
if (! $results) { |
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.
I'd rather see this test here:
if (count($results) == 0) { | |
if ($results === []) { |
This checks for an untouched $results
. As soon as something is in that array we have to assume something worked correctly.
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.
In general I'm not 💯% happy with hiding a multipaged search behind as function as you never know how much resources it will cost. Given larege company directories such a query can return hundrets of thousands of entries. In a not realy memory efficient structure like an array.
So finding a way to return an iterable for this use-case that yields the always current result might be a better approach.
But before we do not have a way to aggregate paged results in such a memory efficient way it is better to use thins approach.
Oh. And I'm also missing some tests here ;-)
$attributes | ||
); | ||
|
||
foreach (ldap_get_entries($resource, $result) as $item){ |
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.
I see an issue here as ldap_get_entries
returns a rather awkward array containing an entry count
followed by a numerically indext list of entries. So for each iteration in the do...while
we will have an entry count
pushed to the results
array. I doubt that is what you had in mind...
); | ||
|
||
foreach (ldap_get_entries($resource, $result) as $item){ | ||
array_push($results, (array)$item); |
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.
This array_push
causes the values of the count
entries to be added multiple times as array.
So you weill have some arbitrary array entries in the result that contain [$pagesize]
...
} while ($cookie); | ||
ErrorHandler::stop(); | ||
|
||
if (count($results) == 0) { |
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.
I'd rather see this test here:
if (count($results) == 0) { | |
if ($results === []) { |
This checks for an untouched $results
. As soon as something is in that array we have to assume something worked correctly.
$$key = $value; | ||
break; | ||
case 'attributes': | ||
if (is_array($value)) { |
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.
What happens when $value
is not an array?
Most of Active directory has a limit of request users like 1k or 5k and if you need to get more users like 10k we need to use search and get users data with pagination. So added Multi page search to the lib. below is the the code to call this function.