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

[Ldap] Add @method addAttributeValues/removeAttributeValues to EntryManagerInterface #30043

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@XuruDragon
Copy link
Contributor

XuruDragon commented Jan 30, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #29433
License MIT
Doc PR n/a

this is the continuity of the PR #29434 of @grahl
Fix should be self-explanatory based on details in #29433

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

we should target master instead of 4.1, otherwise we'll raise a new deprecation, isn't it?

@XuruDragon XuruDragon changed the base branch from 4.1 to master Jan 30, 2019

@XuruDragon XuruDragon force-pushed the XuruDragon:pr-29434 branch from 4a7f0ed to 7278a4c Jan 30, 2019

@XuruDragon XuruDragon force-pushed the XuruDragon:pr-29434 branch from 7278a4c to ac64bc8 Feb 1, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

I'm wondering if adding a separate new interface would be legit?
@csarrazi what your advice here, are you ok with these new @method and adding them in 5.0?
What else?

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 7, 2019

@nicolas-grekas nicolas-grekas changed the title Add @method addAttributeValues/removeAttributeValues to LDAP EntryManagerInterface [Ldap] Add @method addAttributeValues/removeAttributeValues to EntryManagerInterface Feb 7, 2019

@nicolas-grekas nicolas-grekas added Deprecation and removed Bug labels Feb 7, 2019

@XuruDragon XuruDragon force-pushed the XuruDragon:pr-29434 branch from ac64bc8 to ef0342d Feb 7, 2019

@csarrazi

This comment has been minimized.

Copy link
Contributor

csarrazi commented Feb 7, 2019

To be honest, I'm not really a fan of the two methods. In reality, they were only added as part of a workaround because the EntryManager does not handle change sets between old and new entry values, so they should be marked as @internal, or @deprecated with no specific timeline for removal.

As I mentioned earlier, the decision to not add these methods to the interface is because these capabilities are specific to the PHP LDAP extension, and not something which would be available for another implementation. Hence the methods are only present in the ext_ldap-specific EntryManager implementation. Moreover, these methods' signature are identical to the ext_ldap signature, which means that the implementation would be leaking at the interface level, which is not good.

Instead of adding the two methods to the EntryManagerInterface, I would rather introduce a more agnostic method to perform attribute-level operations, such as add/remove/alter one or multiple attribute values. That method would take a list of changes to perform on each attribute. This would help provide a more granular API for such needs, and could be reused later once the component is able to compute change sets for entries.

Then, we could remove the two addAttributeValues and removeAttributeValues methods from the EntryManager.

grahl and others added some commits Dec 2, 2018

Fixes #29433
Coding standard fixes from automated testing.

@XuruDragon XuruDragon force-pushed the XuruDragon:pr-29434 branch from ef0342d to 9b7dada Feb 8, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 8, 2019

Thanks @csarrazi
I'm therefore closing here as "won't fix", thanks @XuruDragon for helping this move forward.
If anyone is willing to implement what's Charles described, PR welcome.

@XuruDragon XuruDragon deleted the XuruDragon:pr-29434 branch Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment