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.Problem/Motivation
As a module developer that wants to take advantage of flag module and implement my own logic of some updates based on flagged/unflagged content, I'd like to subscribe to both events at the same time, and I have no problems with that, but every time one will subscribe to both events, they will have to handle events differently in their code, as the API is inconsistent, FlaggingEvent works with one flagging, while UnflaggingEvent works with an array of flaggings.
Proposed resolution
Drupal core content entities use foreach to iterate over entities in postDelete and send hook_entity_delete separately for every one, I'm wondering if it would be better for DX to do the same for UnflaggingEvent.
Remaining tasks
Provide a patch
User interface changes
None
API changes
UnflaggingEvent constructor accepts array of flaggings entities, after the change it should accept one flagging.
Data model changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | unflaggingEvent_2755683.8.patch | 1.93 KB | socketwench |
| #4 | unflaggingEvent_2755683.4.patch | 1.94 KB | socketwench |
Comments
Comment #2
asgorobets CreditAttribution: asgorobets at FFW commentedI'm happy to provide a patch if that's a valid issue
Comment #3
joachim CreditAttribution: joachim commentedThat does seem like a confusing inconsistency!
Let's see what @socketwench says about this. Also, should be tagged as beta blocker but I'm not sure offhand what our tag for that is.
Comment #4
socketwench CreditAttribution: socketwench at FFW commentedSo the problem is that as it stands, UnflaggingEvent is more versatile as it isn't tied to a particular flag. We only pass the flaggings. We don't use it internally in any other way, however, so we narrow the definition of UnflaggingEvent.
There's also a problem with how the event class is initialized. In Flagging::preDelete(), we're given a list of entities to delete. As a result, we have to either check all flaggings passed to the static method, or assume they're all from the same flag.
Comment #7
joachim CreditAttribution: joachim commentedPatch has whitespace issues, and also doesn't seem to be addressing the subject of this issue?
Comment #8
rbayliss CreditAttribution: rbayliss at Last Call Media commentedYeah, it looks like the preDelete hook can get called for a bunch of flaggings at once. For the sake of consistency, how about something like this instead?
Comment #9
BerdirWe did this on purpose, as a performance improvement. unflag could happen on hundreds of flaggings at once, especially when removing them all to delete a flag.
And flag currently has no concept of multiple, so not sure it makes sense to make that multiple.
I don't really see a problem with them being different.
Comment #10
socketwench CreditAttribution: socketwench commentedJust a reroll for the whitespace issues.
Comment #11
neclimdul"One is a special case of many."
Building the concept of many into the flag API would do a couple improvements IMHO.
1) Make things consistent, solving this issue and preserves the performance improvement.
2) Leave the API open for bulk operations in the future.
Seems like a better option if this is going to happen. If not I'd say this is just won't fix after 3 years of silence and people using the alpha releases.