Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Recent efforts to restore author access functionality has drawn focus to a unnecessary redundancy.
from
https://www.drupal.org/node/2716613#comment-11674285
a) FlagTypeBase looks like a good place for common code WITHOUT assuming the implementation is based on entities.
b) FlagEntityType is a good place for common code based on the assumption that the implementation IS based on entities.
but as Berdir questions
Clearly flag only works for entities as we store an entity type and ID, so those two classes could be merge together?
I agree lets reduce complexity and merge.
As a side note to a side note ... in this light should we find a better place in the namespace tree for FlagTypePluginInterface and FlagTypePLuginManager ?
The issue from which this stems in an alpha blocker... don't know what that makes this issue!
Comments
Comment #2
martin107 CreditAttribution: martin107 as a volunteer commentedPostponing until #2716613: Action access control should be on a per FlagType basis. is fixed.
Comment #3
joachim CreditAttribution: joachim as a volunteer commented> Clearly flag only works for entities as we store an entity type and ID
That wasn't the case on D7, but honestly, I've not heard of anyone flagging non-entities on D7, and on D8 there's no reason to not make something an entity.
Comment #4
Berdirflagging in 7.x-3.0 also has an entity_type and entity_id column, how would you reference something else? Sure, you could stuff in there whatever you want but I doubt that's going to work well?
Comment #5
joachim CreditAttribution: joachim as a volunteer commentedThe name and description don't agree on D7:
You could argue that's the flag type name, and a badly named column ;)
But I'm happy to say that only entities can be flagged is a design decision we already made for 7.x-3.x.
Comment #6
martin107 CreditAttribution: martin107 as a volunteer commented@joachim
can I tentatively declare this an alpha blocker.
The code refactoring is maybe minor, but the
that strikes me as a potentially disruptive change ( disruptive to other contrib modules )
so something best done early when nothing is set in concrete.
This is my preferred directory structure - just to start the ball moving
Comment #7
BerdirSeems to me that moving the interfaces is a separate task?
If we do what the title says (merge flagtypebase into EntityFlagType) then the impact should be minimal as I assume that eventually existing subclasses subclass EntityFlagType and then nothing changes for them.
Comment #8
martin107 CreditAttribution: martin107 as a volunteer commentedA perfectly reasonable thing to say.
Comment #9
joachim CreditAttribution: joachim as a volunteer commented> That wasn't the case on D7, but honestly, I've not heard of anyone flagging non-entities on D7, and on D8 there's no reason to not make something an entity.
I take that back.
Someone just got in touch with me about flagging repeated dates... which are going to be field items maybe?
I have *no idea* how that would even be possible. We've probably gone too far down the route of only working with entities by our use of an entity reference field on Flaggings -- though there's per-bundle base fields overrides IIRC?
Comment #10
socketwench CreditAttribution: socketwench as a volunteer commentedMight be a good 5.x feature?