🇩🇪Germany @jurgenhaas

The way the flag module handles their action plugins is that they use the labels of the flag and unflag links as action labels. Here is my test example, where I have created on flag setup:

They then show in the action plugin list with those names:

Thanks, going to publish a new release now.

It doesn't help much to increase the pressure on maintainers of these projects. They would have got around addressing this if they had the skill, time and budget to get this done. What's really helpful is either skill or funding.

I think I got it: the variable to collect the plugin definitions needs to be static to catch all the scenarios that can lead to the infinite loop. Please review and test if that fixes your scenario as well.

Thanks @rclemings for reporting and analysing this. Much appreciated as always. I've made one little suggestion in the MR that you could just apply in the GitLab UI if that works as expected.

Looks like there isn't any issue. The flag action plugins are not provided by ECA Flag, they are provided by the flag module directly. Therefore, if you scroll down a little further, you should find the missing actions in the "Flag" section:

jurgenhaas → changed the visibility of the branch 3462442-eca-processing-queued to hidden.

Thanks @SirClickalot for reporting this. I've been on vacation and only returned this week. I'll have a look into this now.

My assumption is, that the action plugin from the flag module may have some incompatibility and ECA is just ignoring broken plugins now instead of producing the annoying WSOD.

And the action plugins you're seeing instead are coming from config entities, that exist on your site. When you delete those config entities (probably in the action ui configuration), then they will also disappear from the drop down list in ECA.

Published release 2.0.1

That looks good to me, I'm going to merge and back port this to 2.0.x

With ECA we can access config entities and content entities. That means, for the webform access group, ECA can do something with the data. But for Webform Access Group Membership ECA can't as the table structure doesn't appear to be a content entity, it looks more like an arbitrary database table. For that, a specific action plugin would be required to work with that.

Thanks @eojthebrave for addressing this so quickly. It's difficult to test as I've resolved the issue with the approach from #2.

Just wondering, are you not granting issue credits for reporting and analysing issues?

jurgenhaas → created an issue.

Thanks @fjgarlin, I've also marked all threads in the MR as resolved.

Regarding tests:

• The ECA module uses the upgrade_status test, an example can be found at https://git.drupalcode.org/project/eca/-/pipelines/233596. There is no test with the extra logic yet, since this is covered by the test where the module name gets determined by the info.yml file as well.
• For modules with a different name from the project, I don't have one on d.o but I have a private one:
  • The failing pipeline (https://gitlab.lakedrops.com/apbs/components/entities/-/pipelines/1275495) uses the default templates, and it fails because drush wants to enable the entities module.
  • Then I tested with the MR (https://gitlab.lakedrops.com/apbs/components/entities/-/pipelines/1275500) and there it works as expected. Drush can enable the apbs_entities module.

Thanks for the enhancement. Just a couple of thoughts:

• Not sure if MODULE_NAME is the appropriate name. It's more a PROJECT_NAME as it can be a module or theme or profile, or other things like recipes in the future.
• If a module has sub-modules, there is always more than one. This needs more thought on how to handle that. What's for sure that the requirements of jobs using that information will be different. If a job wants to enable those modules, i.e. the main module and all sub-modules, then a space separated list of all the (sub-)module names is required.
• One could argue that this method should just provide the name of the main module, but then there is that case where a Drupal module has no main module but a few sub-modules.

Is that really a feature for this module? It sounds more like a generic requirement, that certain alerts should be sent to different channels. ECA or DANSE could be great tools to deal with this, but it shouldn't be part of this module. Maybe building some recipes with pre-configured defaults may be better for that sort of requirement?

I've removed the extra checks as suggested. However, let me emphasize this related to

No need to double check that info file exist, one check is enough.

This will now fail for projects with no info.yml file in the root but only for sub-modules. This is because this job is not yet able to deal with sub-modules. It hasn't been before this issue and still doesn't.

Other than that, I've also created the follow-up issue #3463740: Improve MODULE_NAME calculation →

jurgenhaas → created an issue.

there are existing mechanisms for clearing out the queue -- drush, queue ui module

Yes, but site owners may not realize that some constantly failing queue tasks are filling up in their system. Not saying, this is something for ECA, at least not for this issue. But a misconfigured ECA model may fill up queues without anyone noticing it.

I do think another consideration might be whether the user/developer considers the queue to be a reliable one or not

ECA always uses reliable queues, and we should probably stick to that unless someone asks for an option to use unreliable ones. That would be a new feature request then.

We're often using queues so that API calls to remote services are reliable on otherwise unreliable connections (or unreliable services).

Yeah, for that this requeue mechanism is perfect.

Maybe we should do this: log a critical error before throwing the requeue exception. That way, site owners at least get a chance to notice that something went wrong. That would require us the move the new catch block below the 3 lines in EcaAction that already write an error log.

This is very helpful, thank @pearls If you don't mind, I'd like to follow up on this a bit further because it may identify some interesting gotchas.

The stack trace you posted makes perfect sense. All the references and line numbers are accurate, except for one:

#5 /var/www/html/test/web/modules/contrib/ajax_comments/src/Utility.php(101): Drupal\Core\Entity\EntityViewBuilder->view()

The call to EntityViewBuilder->view() is actually on line 96, not 101. Unless you're not using the latest (beta5) release. In beta4 it used to be line 100. Closer, but still not the same. Is it possible that you have modified the file /src/Utility.php in the ajax_comment module, either manually or with a patch? Because in the original state of the file, there is a view mode provided as a string and never a NULL, which is later passed on to ECA by mistake.

If you're certain, that the file has not been modified locally, then there is one other option how that could happen: another contrib module implements the hook entity_view_mode_alter and modifies the given value from 'full' to NULL. If you have a chance, it would be very interesting to know if and where the string _entity_view_mode_alter can be found in the whole code base of your Drupal installation.

Yes, I understand that and it's good that we have that shout out mechanic included so that this creates awareness.

In addition to that, can you agree that the following code is a bug as \Drupal\Core\Field\WidgetInterface::massageFormValues restricts $values to be an array while \Drupal\Component\Utility\NestedArray::getValue returns mixed types and nothing ensure values to be an array?

$values = NestedArray::getValue($form_state->getValues(), $path, $key_exists);
$values = $this->massageFormValues($values, $form, $form_state);

My bad, I've been in an issue fork when I committed that yesterday. Fixed now, it's ready for testing in the dev release.

Perfect, thanks. The Kanban board for the ECA Guide can be found at https://gitlab.lakedrops.com/drupal/documentation/eca/-/boards/321

Note: I've just updated the project page with the new content from the readme file.

Great additional questions, thanks @fjgarlin. I've added comments in the code and also responded to the threads in the MR.

This is now great, and I've merged it into 2.1.x and back ported to 2.0.x

All the outstanding tasks should be addressed in the repo of the ECA Guide at https://gitlab.lakedrops.com/drupal/documentation/eca, see #13, therefore I suggest that we mark this one as fixed.

As I described in #6, core uses $values = $this->massageFormValues($values, $form, $form_state); and assumes, that $values contains an array afterwards. That is a core bug in the first place.

The exception captured in ECA only happens because of that, and I am not convinced that we have any influence over that massageFormValues method to prevent that from happening. Or am I missing something @mxh?

Ah, good one. Thanks @mxh for the hint. I've committed that to the dev branch 2.1.x, @freelock could you give that a try if this also works in your context?

Just returned from vacation and addressed all the great feedback in the MR.

I'll mark this as a task, until we know that this is indeed an ECA bug that is actually fixable.

This is outstanding, thank you so much @ressa for all your effort on this. I've left a couple of minor comments in your MR. We're very close of getting this over the finish line.

The details of the https://ecaguide.org/eca/install can then be addressed in the separate repository for the ECA Guide. If you send me your email address, I can create a user account for you so that we can collaborate on that over there.

This looks pretty good as a staring point. I've left a couple of comments in the MR.

Please, don't set this to NR before the pipeline is green. There are currently a few code style issues, that need to be fixed first.

Are there event plugins that do not extend the EventBase class?

No. So the approach is perfect.

The only concern that comes to mind, if the exception is not temporary but permanent, e.g. due to a misconfigured ECA model, then the item remains in the queue forever, and we don't have any mechanism, to clean up such an issue. Should there be something that controls the max number of retries?

Yes, I saw that too, but that doesn't say that __construct isn't executed.

Still, I can't reproduce the problem. I've created a model that adds a job to the queue. And it also contains a cron event for that job that prints a message. All that works without any error message. What can we do here? I'm really uncertain.

Assigning an issue happens when you select a user name in this drop-down:

Thanks for your feedback, I'm marking this issue as fixed then.

I've fixed the merge conflict which was committed in the code and simplified the solution even further: we can so a search and replace without checking first, if the string is available. @freelock could you please test this once again with your installation? I don't have such a scenario with array values in config and therefore can't see, if this really fixes your issue.

I asked if it could be added in the ecaguide.org instead, what do you think about that?

That would be my preferred route. Adding a section Quick start to the readme, which links to the detailed explanation in the ECA Guide.

I've also reopened that thread in the MR as this isn't resolved yet in my view.

Actually, parent is a field of type entity reference to the term entity. And yes, setting that with the set field value action is exactly what's needed.

Not sure about the status of this issue, though. As you assigned it to yourself, it looks like you're dealing with it on your own, not expecting input from other, right? That field "Assigned" in issue metadata is meant to show, that someone is dealing with it. If that's filled, everyone else assumes, there is nothing to do for them.

I tend to disagree on this one. The hook declaration is not an interface, it's just a documentation file for the API at https://api.drupal.org

The hook_entity_view_mode_alter doesn't declare the argument type in the method itself, but it clearly states in the doc block, that this variable needs to be a string, not NULL. So, if this hook is being called with a NULL value, that's more likely to be an issue of that calling instance.

Unfortunately, we don't know what's calling this hook in this context. @pearls can you please provide the full stack trace? I've looked through the code of ajax_comments but couldn't find where they call entity_view_mode_alter. So, we can only find out with the stack trace, what's causing this issue.

I've commented in the related issue and wonder, how this one isn't a duplicate issue of the other one. Can you explain what the purpose should be of these two separate issues?

As I mentioned in #2, ECA is able to handle all content entity events already. But I don't know how the points module works. If their data is stored in content entities as well, then you should be able to achieve everything needed. If the points module does other things, then some action plugins may be required in the points module.

Somebody who knows the points module should investigate and propose a way forward. I don't have the bandwidth to do that in the foreseeable future, I'm afraid.

Thanks @Glottus for reporting this and then even finding the "resolution".

I agree, we should improve the documentation of the session_user token which can be done in the attributes for the \Drupal\eca\EventSubscriber\EcaExecutionSwitchAccountSubscriber::onBeforeInitialExecution method. Happy to review an MR, if anyone could get around to that.

Using the same token for the switch user action isn't possible, I guess. This is because that action could be called several times, not only once.

I've added a couple comments to the MR. Hope that helps.

Please note, before you set this to NR again, make sure that all tests need to be green first.

Thanks @danielspeicher, merged it and back ported to 2.0.x

Thanks everyone for working on this. I've just merged the fix and will tag a new release 1.1.8 later today.

I'm sure about forward porting this to 2.x, though. I'd rather take advantage of upstream maintenance. The issue here arose due to a BC break in Drupal 10.3, at least in my view. But, as ECA 2 only supports 10.3 or later, we should be save.

Thanks, back ported to 2.0.x

I can't reproduce this issue. And I can't even find any documentation that would explain that the constructor is not called for unserialized objects. Can you please provide more details on how this can be reproduced?

In any event, using \Drupal::something would always create CI pipeline errors. We have to avoid that if we want clean code. So, if this is really an issue, there must be another way of resolving it.

Thanks a lot @Josh Waihi for your patient work on this. I've reviewed it and will merge in a minute.

I don't see this happening. We can't make bpmn_io a dependency of ECA as there will always be users who don't want to use the bpmn_io modeller. And it would be hard to explain, why it still always gets installed for them as well.

The dependency is the other way round: ECA is a dependency for bpmn_io and it is declared there already since day 1.

So, the easy getting started is composer require drupal/bpmn_io and that will install all you need.

What I was saying in my previous comment is that we can rephrase the text on the project page which comes from the README.md in the repository, such that BPMN.io is recognized as "THE MODELLER". Other modellers can be mentioned as options, but we can make clear what people should use by default.

The topic about installation instructions doesn't resonate well on the project page still. Even more so that project browser comes closer. Those instructions will be displayed in project browser to all those new users, but there they will be misleading, or even being wrong. For users with project browser, the installation will just be clicking a button. Such users don't want to know anything about composer require ..., all that happens in the background for them.

In summary, we can re-phrase the sections on the project page where modellers are mentioned. There we can make sure that BPMN.io is recognized as the recommended modeller, i.e. the default modeller if you will. But I don't see download and installation instructions there. As I mentioned before, there are more than 3 different ways of doing it, and describing only 1 of those 3 is wrong in my view.

In other words: the project page is the selling page for the module(s). Documentation belongs to the manuals. We won't mix that.

That method is implemented in \Drupal\Component\Plugin\PluginBase::getPluginId which all plugins extend. So that method should be available. Static code analysis doesn't complain about that missing method, either.

Is there any scenario where you can reproduce that this is causing an exception?

Fixed that in the dev release of 2.1.x - please review and if it's RTBC, I'll be back porting this to 2.0.x

The problem was that none of the consumer entities was defined as default before. No idea, if the default is something new?

What resolved the issue is the following SQL statement in my case:

update consumer_field_data set is_default=1 where id=2;

jurgenhaas → created an issue.

This is great, thank you very much for your contribution.

@aoturoa that's correct. However, we should not assume that each core on a host has the same credentials. It should be working with its individual credentials, if present.

Webforms are not using Drupal's form API, that's why the form events in eca_form are never dispatched when working with a webform.

The eca_webform modules comes with some webform specific event. Maybe the Alter submission form event is working in this scenario? You should then also add some message actions into your model so that you can test if the event is being dispatched and also after the action plugin to see if that action got executed successfully as well.

I had applied a small change when executing the command, so that it shows in the console the URL that needs to be loaded in DRD to finish configuring the agent.

That's a good idea, but it needs a bit more work. Could you please rebase the MR so that it contains the changes from the dev branch and then make sure that it tests first that $values['redirect'] exists? If it doesn't, an error message should be reported, otherwise the success message should explain what to do with the printer redirect URL.

jurgenhaas → created an issue.

I tried testing drush eca:update

That drush command is not involved in any of the update hooks. That's for a different purpose.

What you need to test is to run drush updatedb and you should see, that it will execute the eca_form_update_8003 update hook. For that to work, you don't need to manually edit any yaml file, that should all be done by that hook.

Also, please fix the pipeline error before setting to "Needs review".

This is looking good except for one minor issue. Also, let's do bug fixes against the latest dev release and then back port.

Thanks a lot for your feedback.

The event should implement a setValidationErrorMessage method which can be called by an action plugin called Set validation error or something like that.

The validation plugin, which dispatched the event, should afterwards receive that error message from the event and signal that upstream, if there is any.

This doesn't like an ECA issue. Something accesses the cache collector from Drupal core. That's a core service that receives the cache backend when being initialized. That cache backend is supposed to be NULL according to the error message you receive.

I can only think of some misconfiguration on that Drupal site.

If you doubt that's the case, please provide detailed steps on how to reproduce this issue on a fresh Drupal site.

@cbccharlie the usage of the drushsevices.yml file is deprecated and not required by Drush 12 and later.

However, I just noticed there has been a different issue that prevented Drush from discovering the commands: the class name must end with Commands. I've fixed that in the dev branch. Please have a look.

Do you need to store them? The problem is not about storing them but the fact that the message needs them as a string, when you want to display the list.

If you take the list and then loop over that list, without displaying the list on screen, then you should be fine already, aren't you?

Thanks for the clarification @mxh I was really off-track here.

In order to hook into the validation of TypedData and Entities, we should be able to provide validation constraint plugins for each data type. They should be discovered automatically according to https://www.drupal.org/docs/drupal-apis/entity-api/entity-validation-api... → , and we could dispatch an ECA event, from which a model can take over and do whatever is needed.

This is something that needs to be addressed with a module that provides the email action plugin.

If you're using the email plugin from Drupal core, that only support very simple email functionality. And recipients can only be provided in the To field. There you could send individual emails per recipient, where they don't see their email addresses either.

For more complex email functionality, the easy email module → is recommended. There you can define email templates with almost all supported fields for emails.

Other email modules (e.g.Symfony Mailer) are available too, but I don't have personal experiences with them.

I've committed that to the dev release. Please test and review.

Moving this over to the bpmn_io project since the route bpmn_io.add is defined there and requires the implementation of the title property. The title could be static, e.g. "Create new ECA model with BPMN.io". This could be added as bpmn_io.add.defaults._title

This is caused by the usage of the message list-related = [list-related] which has to Yaml-encode the complex content of the list-related token into a string. As this token contains objects that can't be encoded, this exception is being thrown.

The Yaml encoder comes from Drupal core which uses the Yaml dumper from Symfony. I am not sure if and what they changed in that regard, but serializing a list of entity references is probably something that can and should be avoided.

I've updated the MR. The module name is now determined from the info.yml file name. It also verifies that there is exactly 1 info.yml file available, and aborts otherwise.

we could use code to find the name of the root module from the info.yml

Fair enough, I'll give that a try.

Some modules may have multiple submodules.

That's a great point. However, the situation might be more complex. Some submodules are optional, i.e. they can only be enabled if a certain dependency is available, where that is deliberately not a hard dependency. In such cases, the test can't just enable all submodules. I think, this should be addressed in a separate issue and carefully thought through.

Should this be a hidden variable?

Good question. I'd be OK to make this hidden if that's what we want to go for.

@jsutta thank you for your input. Contributions to this project are very welcome.

In this particular case, the extra feature is not required, as the functionality is already available: for every condition, not only the arbitrary comparison, there is a flag Negate condition. If you set that to Yes, then you already get the "Not equal" condition. That is very flexible because you can use it with all the other comparison types as well, and of course with all other conditions in ECA.

Thank you so much @drumm !!!

This module doesn't implement any action plugins itself. It makes all the tamper plugins from the tamper module → available as action plugins to ECA. So, if such a plugin you're asking for is available in tamper, it will then automatically also be available to ECA. If I don't misunderstand, there is nothing to do for the eca_tamper module.

jurgenhaas → created an issue.

Hmm Drush 11 is EOL since November last year. Why not upgrading to Drush 12?

jurgenhaas → created an issue.

jurgenhaas → created an issue.

Turns out, my workaround only works on PHP 8.1, it fails on PHP 8.2 with reflection class exceptions.

However, that leads towards a proper solution, that I didn't see before: for Drupal core <10.3 we need to remove the attribute usage from the validation plugin and bring back the annotations. That needs to be done both in Drupal core and in the contrib book module.

Then we need the following releases:

• Drupal core: we need 9.2.8 with that being fixed
• Book contrib module: 1.0.1 with that being fixed and a version constraint on Drupal core for >= 10 and <= 10.3
• Book contrib module: 1.1.0 and 2.0.0 with the attributes left in but a version constraint on Drupal core for >= 10.3

As a work around for site owners: use the BookOutlineConstraint.php file from #3 and add the 2 scripts to the root composer.json:

{
  "scripts": {
    "post-update-cmd": "cp config/BookOutlineConstraint.php web/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraint.php && cp config/BookOutlineConstraint.php web/modules/contrib/book/src/Plugin/Validation/Constraint/BookOutlineConstraint.php",
    "post-install-cmd": "cp config/BookOutlineConstraint.php web/core/modules/book/src/Plugin/Validation/Constraint/BookOutlineConstraint.php && cp config/BookOutlineConstraint.php web/modules/contrib/book/src/Plugin/Validation/Constraint/BookOutlineConstraint.php"
  }
}

This works for Drupal core < 10.3

jurgenhaas → created an issue.

jurgenhaas → created an issue.

WRT the real solution, the version constraint for this module should have been that it only works with Drupal core >= 10.3 because the used constraint plugin uses an attribute which isn't available in earlier Drupal core versions. Problem is that this can't be corrected now. Even if there were a new release with that constraint, the 1.0.0 release would still be found by composer and then used for the older Drupal core versions.

For anyone who wants to use the same workaround, attached is a copy of the working constraint plugin, remove the .txt extension after downloading it. Then copy that into the config directory of the project root. In composer.json add 2 scripts:

{
  "scripts": {
    "post-update-cmd": "rm -rf web/core/modules/book && cp config/BookOutlineConstraint.php web/modules/contrib/book/src/Plugin/Validation/Constraint/BookOutlineConstraint.php",
    "post-install-cmd": "rm -rf web/core/modules/book && cp config/BookOutlineConstraint.php web/modules/contrib/book/src/Plugin/Validation/Constraint/BookOutlineConstraint.php"
  }
}

That way it works for now, but it's not a solution to the general problem.

We're running into the same issue on Drupal core 10.2.7. Here is what happens:

• in D10.2.7, the book module is still in core and uses annotations for validation plugins
• there is now also a public release of the book module, which works with all Drupal versions >= 10
• the json_api_book module requires the book module which came from core in the past and should be coming from the contrib module in the future
• because of json_api_book, composer now also installs the book module from contrib
• as a result, we have the book module with the same namespace twice: one time in core and another time in contrib
• the contrib version uses attributes instead of annotations, and that's why Drupal doesn't find the plugins

Workaround, until we can upgrade to Drupal 10.3:

• delete web/core/modules/book after composer update or composer install
• apply a patch to bring back annotations into the validation plugin in contrib

@mlncn that sounds great, let me try getting you going.

The OP talks about the validation handler of an entity type, while you're now mentioning validation plugins. Those are two different things, but we could probably leverage both.

With the validation handler, I could imagine that we implement an ECA condition which takes a content entity as an argument and runs it through its validation handler. It returns TRUE or FALSE, depending on the result of the handler. If it doesn't have one, it falls back to TRUE. Note, there is also always the negation flag for those result, so that should also apply to this result.

With validation plugins, they are applied to distinct values, e.g. field values. So, to utilize a validation plugin, one needs to select the plugin and pass it a value that should be validated. Please correct me if I'm wrong. So, if that is not too far off, then this would qualify for another ECA condition plugin that provides a drop-down list of available validation plugins and a value field. It would then process that combination like described for the handler before.

The entity handler validation plugin should be going into the eca_content sub-module. The other one with validation plugins would be better placed in eca_base, as it could be useful with arbitrary values, not necessarily related to content entities.