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

Fetch the data per page. Consecutive calls will fetch the next page #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jvillafanez
Copy link

Use ->setOperationInvoker(new LdapOperationInvokerForPaging()) to
activate this change. ->setUsePaging(true) is needed to get data per
page, otherwise all the data will be returned.

Example code:

$config = new Configuration();

// A domain configuration object. Requires a domain name, servers, username, and password. 
$domain = (new DomainConfiguration('mydomain.com'))
   ->setBaseDn('dc=domain,dc=com')
   ->setServers(['10.0.2.8'])
   ->setPort(11111)
   ->setUsername('cn=admin,dc=domain,dc=com')
   ->setPassword('password')
   ->setOperationInvoker(new LdapOperationInvokerForPaging())
   ->setUsePaging(true)
   ->setPageSize(250);

$config->addDomain($domain);
// Defaults to the first domain added. You can change this if you want.

$ldap = new LdapManager($config);

$query = $ldap->buildLdapQuery();
$ldap_query = $query->select(['dn'])
   ->where(["objectClass" => "inetOrgPerson"])
   ->setBaseDn('ou=people,dc=domain,dc=com')
   ->getLdapQuery();

while ($users = $ldap_query->getArrayResult()) {
  print_r($users);  // or do other things
}

Note that caching hasn't been tested but I don't think it will work properly, so I guess it should be disabled when it's used with the new operation invoker.

Use  ->setOperationInvoker(new LdapOperationInvokerForPaging()) to
activate this change. ->setUsePaging(true) is needed to get data per
page, otherwise all the data will be returned.
@ChadSikorra
Copy link
Contributor

Thanks for taking the time to work on this! I think the separate OperationHandler is the way to go. There should be one for paging interactively and one for other queries I guess. Though I think the separate operation invoker could be removed. You could add an option to the QueryOperation called pageInteractively or something like that and use that as a check in the supports() method of the OperationHandler. Then you just need to add the operation handler to the existing OperationInvoker. There's also a lot of duplication between the query operation handler and this. We could probably use a trait between the two for certain common things.

Anywho, I still need to take a closer look at this later.

@jvillafanez
Copy link
Author

I think I tried to go with a custom PagedQueryOperation but I discarded that approach because there were too many changes to do, which also involves more testing. There is also the problem of adding like getPagedLdapQuery or something like that to distinguish between a full query and a paged one, which also involves more code changes. Too many changes for someone that just scratched the code's surface 😃

The current approach is pretty much isolated and can be switch on or off just by changing the operation invoker, which I think it's quite handy.

Regarding the duplication, you're completely right. Something left to do. I just wanted to get the things done with the least amount of changes. That's good enough for me at the moment, but feel free to improve it.

@ChadSikorra
Copy link
Contributor

Yeah, as I look back at this I wish I really would have split the query operations into two different ones. One for paging based operations and another for normal non-paged operations sigh :( Now there would be a lot to change to make that happen. Still doable I guess.

Another thing I just realized with this approach is that there doesn't seem to be a way to tell it that paging has ended if you get to a certain result and want to stop. It would also be nice to modify the paging size before the next result set is retrieved (such as in the loop).

I almost feel like a separate getPagedLdapQuery() would be the best route to go here as well. But that would probably require a lot of changes to the existing LdapQuery class, which is already in need of refactoring/improvement :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants