On D7, the flag_flag class had the methods for flagging and unflagging entities. The idea was presumably that this could be overridden by subclasses for behaviour specific to a certain entity type, though in practice this hasn't happened for core entities (it may be done in other contrib modules, but I've not heard of it). It's also worth noting in passing that other flag operations such as trimming and resetting the flag were procedural functions in the module file.

On D8, so far, flagging and unflagging are on the FlagService class, as flag() and unflag() methods.

There are different views on whether this is a good thing or not.

The case for these being part of the service:

  • It's confusing (to me, at least), and buries the API deeper in module than I'd like.
  • If static, the factory method would not be container aware.
  • We would have to make the Flag class container aware, importing ModuleHandler, EventManager, the current user...
  • It'd force the Flag class to do too many things: Be an entity, create flaggings, dispatch events...
  • NodeType does not create Nodes in this fashion; entity_create() calls the EntityManager service to create nodes.

-- socketwench, at https://www.drupal.org/node/2465865#comment-9795351

The case for having these in the flag entity is a bit less well-defined. Myself, martin107, and more recently the author of the patch at #2470645: Port "Trim flag" Rules action to 8.x all instinctively feel methods for 'things that a flag does' should be on the flag. (See also discussion at #2461549: use entity type hinting in routes, but some of that is no longer relevant.)

So I don't think this issue is necessarily going to be about changing any core, but it would be good to have a single place to point people to explain why it's these methods are on the service.

Comments

socketwench’s picture

socketwench CreditAttribution: socketwench commented

Since we can debate the merits of this until the heat death of the Universe, let's just put it to a real world test with a compromise:

Create a convenience method on the Flag entity to call \Drupal::service('flag')->flag() / ->unflag(). We'd then avoid importing everything from the container into the Flag entity, but still have the ease of calling it from the Flag entity. It's a small thing, and wouldn't muck things up for either object.

socketwench’s picture

socketwench CreditAttribution: socketwench commented

On D7, the flag_flag class had the methods for flagging and unflagging entities. The idea was presumably that this could be overridden by subclasses for behaviour specific to a certain entity type, though in practice this hasn't happened for core entities (it may be done in other contrib modules, but I've not heard of it).

In D8 you do not need to subclass the flag entity. You create a new FlagType plugin for your entity type and you can customise it as you like. Interfaces and base classes are provided for this purpose. Same goes for the link type.

Furthermore, if you need to add specific reactions to flag or unflag actions, you can subscribe to the appropriate events.

joachim’s picture

joachim CreditAttribution: joachim commented

> Create a convenience method on the Flag entity to call \Drupal::service('flag')->flag() / ->unflag().

I'd leaning towards thinking we shouldn't do that -- it would mean there's more than one standard way to flag things and would complicate the API.

Also, we're going to have to move getFlagging[s]() to the plugin: see #1133956: Improve efficiency when displaying lists of flaggable comments. I'll file a new issue for that.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Title: consider moving flag()/unflag() methods from FlagService to Flag » Consider moving flag()/unflag() methods from FlagService to Flag
Status: Active » Closed (fixed)

Changes to the outline of the Flag Service have been relatively stable recently.....

Going back to #1

I think we found something we can live with before the heat death of the universe.

We can always open this back up if needed.

(Closed -- well we compromised )