[LDAP] Allow adding and removing values to/from multi-valued attributes #21856
Conversation
c530782 to
7a05d65
Compare
|
ping @csarrazi |
|
I think that we could improve this by introducing a new API for handling changes. Instead of using separate methods for different things, we should rather use a Command pattern, and provide a set of commands which could be implemented without breaking the interface each time we need to implement something. This way, we could have a runCommand() method in the EntryManager, with a list of operations:
This would support the current set, and we could deprecate the other methods afterwards. What do you think about this @nicolas-grekas ? |
|
By the way, this means that the |
|
ping @jean-gui |
|
I was actually waiting for your response to @csarrazi's ping. I'm not sure I have the necessary skills or time to implement this new API though. I'll try to, but am not making any promises. |
|
@jean-gui want me to take over that one? |
|
I won't have much time before next week, though. @symfony/deciders What do you think about the suggestion, btw? |
|
@csarrazi If you can, sure! |
|
Let's see how it goes for 3.4 :) |
|
Status: needs work |
|
@csarrazi Do you think you will have time to work on this one soon? If not, anybody else? |
|
Hi @fabpot. Unfortunately I won't have time to work on this anytime soon. I can spend some time reviewing PRs, but unfortunately I have little time to do anything else. |
|
Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw. |
|
Anyway willing to take over this one? If not, I'm going to close. |
|
We can do that. Like I mentioned before, I no longer contribute to Symfony on a regular basis. One thing though. I would change the method name to removeAttributeValues / addAttributeValues. Second, this PR should be updated to take into account changes in coding standards for Symfony 4 (PHP > 7.1.3 is used, so scalar type hinting can be used now). By the way, |
|
I'll try to do that today. |
03932c6 to
afd02ff
Compare
| * | ||
| * @param Entry $entry | ||
| * @param string $attribute | ||
| * @param array $values |
There was a problem hiding this comment.
The 3 @param entries can be removed (we don't add those if they don't add something that is not part of the PHP method signature already).
| @@ -67,6 +67,47 @@ public function remove(Entry $entry) | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Adds values to an entry's multi-valued attribute from the Ldap server. | |||
|
|
||
| if (!@ldap_mod_add($con, $entry->getDn(), array($attribute => $values))) { | ||
| throw new LdapException(sprintf('Could not add values to entry "%s", attribute %s: %s', $entry->getDn(), | ||
| $attribute, ldap_error($con))); |
There was a problem hiding this comment.
missing dot at the end of the error message.
| } | ||
|
|
||
| /** | ||
| * Removes values from an entry's multi-valued attribute from the Ldap server. |
| * | ||
| * @param Entry $entry | ||
| * @param string $attribute | ||
| * @param array $values |
| if (!@ldap_mod_del($con, $entry->getDn(), array($attribute => $values))) { | ||
| throw new LdapException(sprintf('Could not remove values from entry "%s", attribute %s: %s', | ||
| $entry->getDn(), | ||
| $attribute, ldap_error($con))); |
There was a problem hiding this comment.
should also be on one line, and dot missing at the end of the error message.
|
Done. Should I add those new methods to EntryManagerInterface? Would that be a BC break? |
|
Adding them to the interface would be a BC break. |
|
One last thing before merging: can you add a note about these 2 new methods in the composer CHANGELOG file? |
|
Done (if src/Symfony/Component/Ldap/CHANGELOG.md is the file you meant). I should probably submit a PR for the doc as well. |
|
Thank you @jean-gui. |
…valued attributes (jean-gui) This PR was merged into the 4.1-dev branch. Discussion ---------- [LDAP] Allow adding and removing values to/from multi-valued attributes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT `EntryManagerInterface::update(Entry $entry)` is extremely slow in some specific cases such as adding or removing members to or from huge groupOfNames if you also enable the memberOf overlay in OpenLDAP. Disabling memberOf does make things a lot better, but it is still slow compared to inserting/removing only the elements you want. This PR adds two methods to Symfony\Component\Ldap\Adapter\ExtLdap\EntryManager taking advantage of ldap_mod_add and ldap_mod_del. I thought about using them directly in the update method, but since you need to know what values to add and remove, it would be necessary to retrieve the old values from LDAP. I'm also unsure whether these two methods should be in an interface. I think that adding them to EntryManagerInterface would break BC, but I could create another interface, similarly to RenameEntryInterface. Commits ------- fa9db29 Allow adding and removing values to/from multi-valued attributes
…tributes (jean-gui) This PR was merged into the master branch. Discussion ---------- Documented how to add or remove values of multi-valued attributes Related to symfony/symfony#21856 <!-- If your pull request fixes a BUG, use the oldest maintained branch that contains the bug (see https://symfony.com/roadmap for the list of maintained branches). If your pull request documents a NEW FEATURE, use the same Symfony branch where the feature was introduced (and `master` for features of unreleased versions). --> Commits ------- 40df041 Documented how to add or remove values of multi-valued attributes
EntryManagerInterface::update(Entry $entry)is extremely slow in some specific cases such as adding or removing members to or from huge groupOfNames if you also enable the memberOf overlay in OpenLDAP. Disabling memberOf does make things a lot better, but it is still slow compared to inserting/removing only the elements you want.This PR adds two methods to Symfony\Component\Ldap\Adapter\ExtLdap\EntryManager taking advantage of ldap_mod_add and ldap_mod_del.
I thought about using them directly in the update method, but since you need to know what values to add and remove, it would be necessary to retrieve the old values from LDAP.
I'm also unsure whether these two methods should be in an interface. I think that adding them to EntryManagerInterface would break BC, but I could create another interface, similarly to RenameEntryInterface.