EntityFlagTypeDeriver provides a derived plugin for every entity type, except for a list of ignored types which corresponds to the entity types where we provide a specific plugin class.

This means that a contrib module wanting to provide its own plugin for its entity type wouldn't be able to, because it can't change the hardcoded list of exceptions.

CommentFileSizeAuthor
#10 flagtype-2481793-10.patch6.73 KBgiancarlosotelo
#7 flagtype-2481793-7.patch6.92 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

If you rename the flag type plugins so that they are named after the deriver pattern, then you can drop them, the plugin system will merge them together automatically.

flag_type_node => flag_type_entity:node

You want to keep excluding flagging/flag_type I guess ;)

joachim’s picture

joachim commented

> You want to keep excluding flagging/flag_type I guess ;)

I'm not actually sure we do: flag_type will get excluded by #2409575: config entity types should not be flaggable, and flagging flaggings is a bit weird but there could be use cases...

Thanks for the pointers :)

socketwench’s picture

socketwench commented

I never liked the $ignoredEntities system. It always seemed like a stop gap then, but I didn't have a better idea.

This morning I'm wondering if what we can do is add a method to the FlagTypeInterface to list the supported types with a weight. Then we can enumerate all the plugins passing them a list of all entities. Then each plugin can suggest the types it supports by attaching themselves to the entity type in the list. Then the plugin manager can use the weights to sort out who handles what. The EntityFlagTypeDeriver can suck up whatever's left, removing config entities.

joachim’s picture

joachim commented

That sounds a little over-engineered to me. According to Berdir in #1, we can just name the plugins a certain way and the core plugin manager should know to override the basic one.

socketwench’s picture

socketwench commented

I find relying on naming a little obscured in this case. I'd rather do a little exploratory coding before I mark this won't fix.

joachim’s picture

joachim commented

Berdir makes it sound like a documented part of the plugin system, and indeed the way derived plugins are meant to work.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new flagtype-2481793-7.patch6.92 KB

I'm not sure about documented, but it's certainly a pattern that is used in core.

I actually think it's quite clear, it's a specialized form of the generic entity flag type.

Since this means we have to rename the plugins anyway which makes this an API change, maybe this would be a good moment to get rid of the strange flagtype_ prefix in the plugin ID's? Then it's just entity:node for example.

joachim’s picture

joachim commented

> Since this means we have to rename the plugins anyway which makes this an API change, maybe this would be a good moment to get rid of the strange flagtype_ prefix in the plugin ID's? Then it's just entity:node for example.

Yeah, that makes sense.

Plugins IDs are usually seen in the context of their plugin type, eg 'flag.flag_type.plugin.flagtype_entity:user', so the second 'flagtype' is redundant.

socketwench’s picture

socketwench commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
giancarlosotelo’s picture

giancarlosotelo commented
Status: Needs work » Needs review
StatusFileSize
new flagtype-2481793-10.patch6.73 KB

Rerolled and also removed the 'flagtype' where was needed included 'flagtype_entity'.

socketwench’s picture

socketwench commented
Status: Needs review » Reviewed & tested by the community

Applies and tests clean.

Random idea: What if instead of hardcoding exclusions, we create a "/dev/null" plugin that would prevent use? That way, modules could still provide a flagtype plugin for excluded plugins.

martin107’s picture

martin107 commented
Issue tags: -Needs reroll
joachim’s picture

joachim commented

Haven't we just about removed all the exclusions? Either that, or there are issues for that floating around. I certainly remember thinking we'd be removing the exclusion of 'flagging' and letting people tie themselves in knots if they want to ;)

socketwench’s picture

socketwench commented

Haven't we just about removed all the exclusions?

Probably, I've been so busy this summer I've had a hard time keeping track. Specifically, I was thinking of config entities, since I can imagine someone coming up with a good use case, even if we can't.

joachim’s picture

joachim commented

> Specifically, I was thinking of config entities, since I can imagine someone coming up with a good use case, even if we can't.

Let's cross that bridge if/when we come to it. If someone posts an issue saying they need us to be less draconian about excluding config entities, we can do what you suggest, or incorporate whatever they propose. Until then, I think this is quite probably a YAGNI.

joachim’s picture

joachim commented

> Haven't we just about removed all the exclusions?

+++ b/src/Plugin/Derivative/EntityFlagTypeDeriver.php
@@ -26,9 +21,6 @@ class EntityFlagTypeDeriver extends DeriverBase {
   protected $ignoredEntities = [
     'flagging',
-    'node',
-    'user',
-    'comment',
   ];

Once this patch is in, the only entity type being excluded is flaggings. Removing that is the subject of #2481795: unnecessary flagging in EntityFlagTypeDeriver::$ignoredEntities.

joachim queued 10: flagtype-2481793-10.patch for re-testing.

joachim’s picture

joachim commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks everyone!

  • joachim committed dc27dcd on 8.x-4.x authored by Berdir 
    Issue #2481793 by Berdir, giancarlosotelo: Fixed flag type plugin IDs so...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.