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
FlagServiceInterface::getFlaggings() may take a large variation of parameters. While this works, it is seen as bad DX and requires a nuanced understanding of the method operation.
Proposed resolution
Define additional methods that accept a required number of parameters. These new methods may rely on getFlaggings(), or perform their own entity queries. In the latter case, getFlaggings() should be removed.
Remaining tasks
Create patch.
User interface changes
None.
API changes
Additional flag service methods for retrieving flaggings would be available. FlagServiceInterface::getFlaggings() may no longer be available.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | flippyFloppy_2747833.4.patch | 14.3 KB | socketwench |
| #2 | flippyFloppy_2747833.2.patch | 14.28 KB | socketwench |
Comments
Comment #2
socketwench CreditAttribution: socketwench at FFW commentedRemoved FlagService::getFlaggings() and FlagService::unflagAll() and replaced them with several methods.
Comment #3
martin107 CreditAttribution: martin107 commentedI think this moves us in a positive direction. so +1 from me.
All the changes look good with one minor quibble.
My idiot brain did a "What the what now!" and went running for the documentation...when coming across the method name unflagAllFlag
I suggest renaming it to FlagService::unflagAllByFlag($flag)
I am out of time tonight - I am posting early because naming things is hard and others are welcome to take a different perspective.
I will post a patch tomorrow.
Comment #4
socketwench CreditAttribution: socketwench at FFW commentedRenamed the methods.
Comment #7
martin107 CreditAttribution: martin107 commentedThanks for doing that,
socketwench++
The test failing from an unrelated flag patch look similar.
https://www.drupal.org/node/2755683#comment-11346979
Comment #8
socketwench CreditAttribution: socketwench at FFW commentedRetesting, it looks like the branch errors bit us.
Comment #9
martin107 CreditAttribution: martin107 commentedI think this is good to go.
Patch looks good.
I have seen unease expressed by others in separate issues about how digging through over complex methods leaves a bad smell.
There is a lot of care and thought here, to make the DX cleaner. and to my mind answer all the questions.
sockerwench++
Comment #10
socketwench CreditAttribution: socketwench at FFW commentedThis is one of those patches that I wrote, I don't like, but others think it's improvement... So...