Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[LDAP] Allow adding and removing values to/from multi-valued attributes#21856
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
c530782 to7a05d65Comparenicolas-grekas commentedMar 4, 2017
ping@csarrazi |
csarrazi commentedMar 5, 2017
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 ? |
csarrazi commentedMar 5, 2017
By the way, this means that the |
nicolas-grekas commentedApr 17, 2017
ping@jean-gui |
jean-gui commentedApr 18, 2017
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. |
csarrazi commentedApr 18, 2017
@jean-gui want me to take over that one? |
csarrazi commentedApr 18, 2017
I won't have much time before next week, though. @symfony/deciders What do you think about the suggestion, btw? |
jean-gui commentedApr 18, 2017
@csarrazi If you can, sure! |
nicolas-grekas commentedApr 18, 2017
Let's see how it goes for 3.4 :) |
nicolas-grekas commentedSep 26, 2017
Status: needs work |
fabpot commentedOct 1, 2017
@csarrazi Do you think you will have time to work on this one soon? If not, anybody else? |
csarrazi commentedOct 2, 2017
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. |
nicolas-grekas commentedOct 8, 2017
Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw. |
fabpot commentedMar 31, 2018
Anyway willing to take over this one? If not, I'm going to close. |
jean-gui commentedApr 3, 2018
csarrazi commentedApr 4, 2018
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, |
jean-gui commentedApr 4, 2018
I'll try to do that today. |
03932c6 toafd02ffCompare
fabpot left a comment
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.
some small comments (mainly coding standard).
| * | ||
| * @param Entry $entry | ||
| * @param string $attribute | ||
| * @param array $values |
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.
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).
| } | ||
| /** | ||
| * Adds values to an entry's multi-valued attribute from the Ldap server. |
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.
LDAP
| if (!@ldap_mod_add($con,$entry->getDn(),array($attribute =>$values))) { | ||
| thrownewLdapException(sprintf('Could not add values to entry "%s", attribute %s: %s',$entry->getDn(), | ||
| $attribute,ldap_error($con))); |
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.
should be on one line.
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.
missing dot at the end of the error message.
| } | ||
| /** | ||
| * Removes values from an entry's multi-valued attribute from the Ldap server. |
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.
LDAP
| * | ||
| * @param Entry $entry | ||
| * @param string $attribute | ||
| * @param array $values |
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.
same as above
| if (!@ldap_mod_del($con,$entry->getDn(),array($attribute =>$values))) { | ||
| thrownewLdapException(sprintf('Could not remove values from entry "%s", attribute %s: %s', | ||
| $entry->getDn(), | ||
| $attribute,ldap_error($con))); |
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.
should also be on one line, and dot missing at the end of the error message.
jean-gui commentedApr 4, 2018
Done. Should I add those new methods to EntryManagerInterface? Would that be a BC break? |
fabpot commentedApr 4, 2018
Adding them to the interface would be a BC break. |
fabpot commentedApr 4, 2018
One last thing before merging: can you add a note about these 2 new methods in the composer CHANGELOG file? |
jean-gui commentedApr 4, 2018
Done (if src/Symfony/Component/Ldap/CHANGELOG.md is the file you meant). I should probably submit a PR for the doc as well. |
fabpot left a comment
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.
A PR on the docs would be the cherry on the cake :)
fabpot commentedApr 4, 2018
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 attributesRelated tosymfony/symfony#21856<!--If your pull request fixes a BUG, use the oldest maintained branch that containsthe bug (seehttps://symfony.com/roadmap for the list of maintained branches).If your pull request documents a NEW FEATURE, use the same Symfony branch wherethe 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.