Skip to content

ldap query minor refactoring #28459

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

ldap query minor refactoring #28459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e73dad
ldap query minor refactoring
ronfroy Sep 13, 2018
f9413e0
fix
ronfroy Sep 13, 2018
21fb399
revert
ronfroy Sep 17, 2018
185ffd1
revert
ronfroy Sep 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Symfony/Component/Ldap/Adapter/AbstractQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ public function __construct(ConnectionInterface $connection, string $dn, string
'maxItems' => 0,
'sizeLimit' => 0,
'timeout' => 0,
'deref' => static::DEREF_NEVER,
'deref' => QueryInterface::DEREF_NEVER,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break backwards compatibility, as you could use them in a child implementation and overwrite them. @csarrazi do you know if that was intentional?

Copy link
Contributor Author

@ronfroy ronfroy Sep 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is, but it's a design fix too. Const override is really not a good practice, and const must not be part of the interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a BC break nevertheless. The question is whether this is ever used, but we can't check that. Perhaps on initialization of the object you can do something like this?

public function __construct(...)
{
    if (static::DEREF_NEVER !== QueryInterface::DEREF_NEVER) {
        @trigger_error('...', E_USER_DEPRECATED);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iltar I can not make it deprecated and fixed it in the same time. But i dont think this is ever used.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can trigger the deprecation, which should cause developers to fix the issue. It'll have to be fixed in 5.0. It's up to the core members to decide whether or not this is an acceptable BC break

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, one of the things I'm unsatisfied with is the fact that the values in QueryInterface are actually extracted from the ext_ldap implementation, which is actually problematic as this means that the implementation leaks into the interface.

This was actually the rationale behind using static:: instead of QueryInterface, as the goal was that other implementations could use other values.

For 5.x, the actual values used in the QueryInterface should instead be defined in the actual implementation instead of the interface, and the interface constants should rather be string values.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally not a fan of having constants in an interface, because it spreads to every implementation. Would it be an idea to make a final class with private constructor to limit the constants to 1 class instead?

Copy link
Contributor

@csarrazi csarrazi Sep 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I agree. We should create a LdapOption, which would contain string values for each constant.
The actual value (used by ext_ldap) should be limited to the actual implementation, so should only be present in the query implementation.

'attrsOnly' => 0,
'scope' => static::SCOPE_SUB,
'scope' => QueryInterface::SCOPE_SUB,
));
$resolver->setAllowedValues('deref', array(static::DEREF_ALWAYS, static::DEREF_NEVER, static::DEREF_FINDING, static::DEREF_SEARCHING));
$resolver->setAllowedValues('scope', array(static::SCOPE_BASE, static::SCOPE_ONE, static::SCOPE_SUB));
$resolver->setAllowedValues('deref', array(QueryInterface::DEREF_ALWAYS, QueryInterface::DEREF_NEVER, QueryInterface::DEREF_FINDING, QueryInterface::DEREF_SEARCHING));
$resolver->setAllowedValues('scope', array(QueryInterface::SCOPE_BASE, QueryInterface::SCOPE_ONE, QueryInterface::SCOPE_SUB));

$resolver->setNormalizer('filter', function (Options $options, $value) {
return \is_array($value) ? $value : array($value);
Expand Down
23 changes: 11 additions & 12 deletions src/Symfony/Component/Ldap/Adapter/ExtLdap/Query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Ldap\Adapter\ExtLdap;

use Symfony\Component\Ldap\Adapter\AbstractQuery;
use Symfony\Component\Ldap\Adapter\QueryInterface;
use Symfony\Component\Ldap\Exception\LdapException;
use Symfony\Component\Ldap\Exception\NotBoundException;

Expand Down Expand Up @@ -62,20 +63,18 @@ public function execute()

$con = $this->connection->getResource();

switch ($this->options['scope']) {
case static::SCOPE_BASE:
$func = 'ldap_read';
break;
case static::SCOPE_ONE:
$func = 'ldap_list';
break;
case static::SCOPE_SUB:
$func = 'ldap_search';
break;
default:
throw new LdapException(sprintf('Could not search in scope "%s".', $this->options['scope']));
$funcMap = array(
QueryInterface::SCOPE_BASE => 'ldap_read',
QueryInterface::SCOPE_ONE => 'ldap_list',
QueryInterface::SCOPE_SUB => 'ldap_search',
);

if (!isset($funcMap[$this->options['scope']])) {
throw new LdapException(sprintf('Could not search in scope "%s".', $this->options['scope']));
}

$func = $funcMap[$this->options['scope']];

$this->search = @$func(
$con,
$this->dn,
Expand Down