Dear flag maintainers: we're working on a new rules engine for Drupal 9+ and the community has already requested some integration with flag. We could of course do that in a separate eca_flag module, but since the integration is really non-invasive we would love to put that into the flag module directly. If you don't mind, we'd provide a MR for this soon.

Issue fork flag-3238783

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jurgenhaas created an issue. See original summary.

holybiber’s picture

holybiber
Location Berlin
commented

That would be great! This would also bring a good solution for bringing back the "threshold" functionality (see https://www.drupal.org/project/flag/issues/2540038) which I'm very interested in and would help with making it happen.

holybiber’s picture

holybiber
Location Berlin
commented

Would you already have a patch or merge request so that I could try this out?

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

We want to get ECA across the beta-1 line first next week, before we start building integrations. But that should happen fairly quickly afterwards.

phily’s picture

phily
he/him
Primary language French
Location 🇪🇺🇫🇷 Paris, France
commented

Hi there,
Any updates about Flag & ECA integration?
Thanks

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

@PhilY not yet, unfortunately. We've been busy with ECA and BPMN.io for the last few months and haven't had the bandwidth to look into third party integrations as well. But we're planning to focus more on that moving forward.

dries arnolds’s picture

dries arnolds
Primary language Dutch
Location 🇳🇱 Amsterdam
commented

I think flag/eca integration is an important one. Flag gives sitebuilders the opportunity to have visitors or editors mark any entity and with ECA it gives a lot of tools to fight abuse, fight spam, do maintenance, vote, and help out in a lot of other useful ways.

rodmarasi’s picture

rodmarasi commented

follow

dries arnolds’s picture

dries arnolds
Primary language Dutch
Location 🇳🇱 Amsterdam
commented

There's a follow feature (star) right below the issue details on the right.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

FYI, started working on ECA Flag module. It's not functional yet but should be getting there soon. Trouble being, that the flag module is using deprecated classes like e.g. Symfony\Component\EventDispatcher\Event which is not compatible with ECA. Therefore, again the question: are there Flag maintainers around who would want to work with us on getting all this sorted?

With regard to the flag actions, we're working on that generally in #3279023: Add ability to execute action configuration entities. That's not finally decided yet, if the approach in MR !203 will be actually used, but I wanted everybody to know, that things are progressing.

demon326’s picture

demon326 commented

@jurgenhaas thanks for taking your time to create a module that works with flag to automate stuff in the future.

I fear that the many maintainers just have more interesting projects to handle than the flag project.. This project needs a dedicated maintainer as its getting more popular as sites upgrade to drupal 8 and beyond.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> Trouble being, that the flag module is using deprecated classes like e.g. Symfony\Component\EventDispatcher\Event

I'll get to making this project D10 compatible eventually, has not been a priority so far. There are issues like #3272521: Drupal 10 compatibility.

> This project needs a dedicated maintainer as its getting more popular as sites upgrade to drupal 8 and beyond.

I'm open to additional maintainers, But there's also plenty people can do without maintainer access like reviewing and testing issues. I'm the only one who is even remotely active. That said, the numbers don't really support that statement, D8 usage has barely increased in the last 6 month: https://www.drupal.org/project/usage/flag

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

@Berdir there is the main issue with \Drupal\flag\Plugin\Action\FlagAction::__construct which assumes that the flag action will only be used in a preconfigured context. the problematic line is this:

    $this->flag = $this->flagService->getFlagById($configuration['flag_id']);

If this is being used with no configuration, e.g. when calling the Drupal core action plugin manager to get a list of all plugins, then $configuration['flag_id'] is undefined and hence getFlagById will be called with NULL as the argument. On PHP 7.4 and beyond, this throws an un-catchable exception.

As a first measure, if you could change that line and its follower to this

    $this->flag = $this->flagService->getFlagById($configuration['flag_id'] ?? 'flag');
    $this->flagOperation = $configuration['flag_action'] ?? '';

then this would at least prevent the exceptions and WSODs for users. They always occur when flag is just enabled together with either VBO (views bulk operations) or ECA (Event - Condition - Action).

@holybiber @Dries Arnolds @rodmarasi @PhilY @demon326 as you can see in the issue queue, there is currently a lot going on with the flag module and I tend to postpone the ECA integration until things have settled as it doesn't really make a lot of sense, to change things at two ends at the same time.

jurgenhaas opened merge request !28

jurgenhaas opened merge request !29

jurgenhaas closed merge request !28

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

I have created !MR29 and you can apply the patch from https://git.drupalcode.org/project/flag/-/merge_requests/29.diff so that you can at least have Flag and ECA installed at the same time until this is being addressed in the flag module.

phily’s picture

phily
he/him
Primary language French
Location 🇪🇺🇫🇷 Paris, France
commented

@jurgenhaas: sometimes, a 2 lines fix makes people happy ;-)
Thanks a lot for taking time to commit patch @ #17.
WSOD is gone, both modules can be enabled at the same time and as I don't have the need yet for ECA to react upon Flag event, cheers!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That change doesn't seem right. a flag id is a config entity, you can't just hardcode that to flag. I'm confused why getting a "list of plugins" would instantiate them, do you really get plugins or plugin definitions?

A better fix would probably be to drop those lines entirely and directly read the necessary values from $this->configuration when executed?

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

Not exactly sure what the correct resolution would be, I was just trying to fix the WSOD.

When building a list of actions, this is a simplified version of what's been called:

foreach ($this->actionPluginManager->getDefinitions() as $plugin_id => $definition) {
  $actions[] = $this->actionManager->createInstance($plugin_id);
}

So, this is getting a list of action plugins.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Right, but what do you need plugin instances for? that looks very expensive. what does a plugin give you that you can't get from $definition at that point?

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

We need the plugins to e.g. build configuration forms.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes, sure, but you're not building the configuration form for all the plugins at once, you're doing it just for one?

The usual workflow with plugins is that you get the definitions, use that to display a list of them, and then create an instance of one of them configure it or do stuff with it.

Then at least it would only fail if someone were to actually chose the flag action plugin.

Again, to be clear, that code is definitely bad and needs to be improved, just not like that.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

We have to build configuration form information for external (non-Drupal) clients, where we can't build them just in time. For those situations, we have to build them all at once, but we don't do that often, as we cache the results.

mxh made their first commit to this issue’s fork.

mxh opened merge request !30

mxh’s picture

mxh commented
Status: Active » Needs review

I've had another idea how the action plugin instance may be initialized, by using information from the plugin deriver. I've put that idea into MR30, hope this is not getting too complicated now.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I will need to have a look at the overall code, but that seems sensible, I didn't realize they were derivates. Not sure we need a fallback at all. existing places that have the config would still work and the next cache clear, we can expect the plugin definition to be there.

Can we move that to a separate issue? While this may break ECA, it's not actually related to integrating with it and it's weird committing it in scope of this issue.

mxh’s picture

mxh commented

Not sure we need a fallback at all. existing places that have the config would still work and the next cache clear, we can expect the plugin definition to be there.

Not sure either. Was just thinking about an approach with the least probability of breaking existing usages on API level.

Can we move that to a separate issue?

Sure, move it or rename it as you need.

Anonymous’s picture

Anonymous (not verified) commented

I patched the Flag module with the patch mentioned in #26, and flagging of a node works as expected as an ECA event and throws no errors.

I have 12 flags enabled on my site. These have a range of functions. All appear to still work as expected since applying the patch.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

In our case, the error comes from Views bulk operation


Warning: Undefined array key "flag_id" in Drupal\flag\Plugin\Action\FlagAction->__construct() (line 62 of modules/contrib/flag/src/Plugin/Action/FlagAction.php).
Drupal\flag\Plugin\Action\FlagAction->__construct(Array, 'flag_action:save_unflag', Array, Object) (Line: 74)
Drupal\flag\Plugin\Action\FlagAction::create(Object, Array, 'flag_action:save_unflag', Array) (Line: 21)
Drupal\Core\Plugin\Factory\ContainerFactory->createInstance('flag_action:save_unflag', Array) (Line: 83)
Drupal\Component\Plugin\PluginManagerBase->createInstance('flag_action:save_unflag', Array) (Line: 127)
Drupal\views_bulk_operations\Service\ViewsBulkOperationsActionProcessor->initialize(Array) (Line: 167)
Drupal\views_bulk_operations\Service\ViewsBulkOperationsActionProcessor->getLabels(Array) (Line: 64)
Drupal\views_bulk_operations\Form\ConfirmAction->addListData(Array) (Line: 44)
Drupal\views_bulk_operations\Form\ConfirmAction->getFormData('content', 'admin_content') (Line: 77)
Drupal\views_bulk_operations\Form\ConfirmAction->buildForm(Array, Object, 'content', 'admin_content')
call_user_func_array(Array, Array) (Line: 531)
Drupal\Core\Form\FormBuilder->retrieveForm('views_bulk_operations_confirm_action', Object) (Line: 278)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 31)
Drupal\jsonapi_earlyrendering_workaround\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 159)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 81)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 67)
Drupal\simple_oauth\HttpMiddleware\BasicAuthSwap->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 49)
Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\http_headers_cleaner\Middleware\HttpHeadersCleanerMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 709)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Patch from MrRsolved it.

mxh’s picture

mxh commented

Created a specific issue as suggested in #28 for the plugin definition approach: #3322747: Action plugins cannot be instantiated in a generic way

mxh closed merge request !30

mxh’s picture

mxh commented
Status: Needs review » Fixed

#3322747: Action plugins cannot be instantiated in a generic way got fixed, which should have fixed this issue too.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Fixed » Active

The reason I requested a separate issue is that I expect that there might be more things that would be useful for an integration, not just that bugfix, so lets keep this open for now?

sirclickalot’s picture

sirclickalot
Primary language English
Location Somerset
commented

Sorry folks, I'm going around in circles!
Can someone please tell me where precisely I can find the patch for the Flag module to make it work with ECA.
I have followed various others' comments about "...the patch in #26..." but I cannot fidn the actual patch file.
Thanks

dries arnolds’s picture

dries arnolds
Primary language Dutch
Location 🇳🇱 Amsterdam
commented

It came through in a merge request. You can just download the dev version I think.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

@Berdir regarding your comment in #36 it looks like Flag and ECA are now working together nicely. The only missing piece in the flag module to make the collaboration of the two meaningful is the token support, which is addressed in #2500091: Re-implement hook_tokens(). With the latest patch from there it looks as if things are OK and this one could be closed.

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

ECA Flag integration Beta-1 is now available: https://www.drupal.org/project/eca_flag

ivnish’s picture

ivnish
he/him
commented
Status: Active » Postponed (maintainer needs more info)

jurgenhaas, what the status of this issue? Is it relevant?

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Postponed (maintainer needs more info) » Needs review

The integration between ECA and flag is being done in the separate eca_flag module and working nice. But the fix in MR!29 should still be added to the flag module, as it fixes the action plugin in this module which uses a config value that's not always been set and therefore raises a warning. It's an easy fix, so I think it can be merged with any risk.

joachim’s picture

joachim commented

Note that if you're just using a flag as a UI to trigger the ECA action and you don't care about the flagging entity that doing that creates, you'd be better off using https://www.drupal.org/project/action_link instead.

mxh’s picture

mxh commented

But the fix in MR!29 should still be added to the flag module, as it fixes the action plugin in this module

Maybe I'm wrong as this is long time ago. But since #3322747: Action plugins cannot be instantiated in a generic way is in for a while now, which is supposed to have the problem fixed already, I don't think MR29 is necessary anymore.

Note that if you're just using a flag as a UI to trigger the ECA action and you don't care about the flagging entity that doing that creates, you'd be better off using https://www.drupal.org/project/action_link instead.

Interesting module. Just a side note, ECA already has anything included to achieve the same thing, e.g. using "Render custom form" with a submit button and adding an Ajax handler to it. Or create a regular link with "Render: link" and build an ECA endpoint for it.

ivnish’s picture

ivnish
he/him
commented
Status: Needs review » Fixed

Thanks all, MR29 committed!

  • ivnish committed 6d1f6470 on 8.x-4.x 
    Revert "Issue #3238783: Integrate with ECA"

This reverts commit...
ivnish’s picture

ivnish
he/him
commented
Status: Fixed » Needs work

I explored the same issue.

@jurgenhaas how do you think, do that change help you with ECA?

https://git.drupalcode.org/project/flag/-/merge_requests/31/diffs

Now, I reverted this issue commit

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented
Status: Needs work » Closed (duplicate)

@ivnish as @mxh wrote in #43, this MR is no longer required since #3322747: Action plugins cannot be instantiated in a generic way fixed the issue already in a different way. Closing as duplicate.