🇩🇪Germany @jurgenhaas

Ah, sorry, then I misread that.

But I guess we should be able to get both. See this change record → which comes with an option "Object method, instantiated by ClassResolver, container aware using ContainerInjectionInterface", the third from the bottom of the list. That should allow us to use the method in GinSettings with a non-static method and dependency injection.

Looks much better thank you. Any reason why we want to keep _gin_user_form_submit around? Couldn't the form builder refer to the submit method in the service directly? Just one fewer step that might otherwise be confusing at some point.

Yeah, it really sounds like the community is comming together behind this Klaro module. That's what I was told all over DrupalCon last week.

Here is also a link to our ADR on how the decision came together that Drupal CMS goes with Klaro: https://git.drupalcode.org/project/drupal_cms/-/wikis/Architecture-Decis...

it sounds like there currently there isn't a way to include a link to the entity

That's not what I'm saying. There are tokens around at the point of rendering, I just don't know which ones, and I don't know about your context. Notifications can be about very different entity types, etc. I was just trying to explain that the "current-page" can certainly not be the correct token. Everything else needs some research and playing around with the tool, but I currently don't have the bandwidth to do that.

Well, I was only giving an abstract example. The real token syntax depends on what data is available to the token service at that point. And that determines the token name then that needs to be used. But again, we've never done that in our projects, so I'm just trying to give hints on where to look at.

jurgenhaas → created an issue.

The token [current-page:url] is the current page, but you want the URL of your entity. So, you should look into the token browser to find out how the token for that entity looks like. Maybe something like [node:url].

And HTML emails is a challenge, it needs some experimentation to get this to work. We've not enabled that at our client's sites yet.

It's in the extra section (third column) at the very bottom.

Sure, supporting the batch API sounds very attractive. I'm just worried that the batch API only works in a fairly limited context and not across the board. Should this be implemented as a special route in the eca_endpoint sub-module? There we can ensure the proper context and let it work with multiple http requests in a row that make sure that none of the individual requests runs into a timeout.

This is an interesting thought. However, we've achieved re-usable render components by adding them to tokens and then add those tokens to the render array. Or in other use case, e.g. for views fields or extra fields on entities, we've just started with multiple events leading to the same render actions to provide the same field for multiple use cases.

Of course, making this even more convenient is a good thing. Maybe even with a "Custom render event"?

Great analysis, thanks @christianadamski, it sounds perfectly correct. I wonder how the response event sneaked in here, this should more likely be the Controller found to handle request event. If that turns out to be true, we should just update the sample model in the ECA Guide, that's why I've changed the status from bug report to task. But I'm waiting for your confirmation before making any changes there.

That's 2 different events, isn't it. One of them is eca_render.entity (with the red line) and eca_views.pre_execute with the green line). The tokens are provided by the events, though. The pre_execute provides the view arguments, the render event doesn't. And the scope of tokens is only always within the chain of the current event, never outside of that.

Left a few comments in the MR.

While I like the new plugin ViewsSetPagination, I think this is a nice separate new feature, but it should be the "solution" for the issue with subsequent pager requests. The reason being it that this would only work if one has a complex ECA model always around which fixes the issue with the pager. That's not sustainable.

We need to look into the rendered view as prepared by the render views plugin and see why this isn't working across multiple ajax pager requests. We need to get this to work without any extra events to hook into the logic. If core renders that view with a pager, it lets us page through without any issue, the same needs to be achievable by "custom" code, i.e. from within ECA.

This looks great, thanks @luke.leber for your contribution. Just a minor suggestion in the MR, everything else is ready to be merged.

That sounds like the config on that page is broken even before anything doing with ECA. You can verify this by executing a drush cex and a drush cim, that will show you issues with the config on that site, that has nothing to do with ECA import or export.

Please test the MR and let us know if that fixes your issue.

Agreed, @killah89 is right. That setting to globally execute ECA as a specific user is not longer considered a good idea. It comes with too many negative side effects. Switching user explicitly when needed is much better.

And on a separate note: when you start with the "Update entity" event and then update that entity again, you run into a recursion. That's what you also want to avoid. You should instead start with the "Presave entity" event.

Building the entity, then reading the field value into a token, which will be holding a list. Then you can loop through the list and check each value individually.

Agreed, this will make a lot of tings much easier. When it comes to the syntax on how to express this in an ECA config entity, I suggest we do that in relation to the ✨ Improve gateway support Active issue to cover them all.

Thank you, this looks perfect.

This is looking good now. Only a tiny clean-up is left to remove 2 obsolete lines of code.

After that, only tests are required. They should at least test a couple of list merges and also test the cases that would not be executed due to the access control.

I'd say, that doesn't help much. People who find the Klaro module have already found what they need. It's more like the other modules should point to Klaro with a recommendation to use this instead.

The EU Cookie Compliance module already has that recommendation in bold at the top of their project page.

And the GDPR module makes one assume it does the same, but it has no consent management whatsoever. And the module itself doesn't seem to be much maintained these days.

The hook is executed by drush deploy:hook command.

OK, so I have no idea how many people will actually get that. It's not an official way to update modules, that should only rely on core hooks.

The way you describe the update process in two steps for the future sounds about right. As for this time, I'm not sure you need to change that again. There are just 25 reported sites using the module so far, and I have fixed mine manually. But I can't tell about the others.

And again, which ever way, you should not introduce a new dependency and already use it in a service. Whatever your users are doing, they will most likely not be able to install that new dependency because the site breaks.

My question still is, where is that deploy hook coming from? I mean, what is executing this hook?

This can not move on until the questions have been discussed and brought to a conclusion.

This looks great, thanks @jfauske for your help on this one.

You need to enable this plugin in the advanced view settings under /admin/structure/views/settings/advanced

@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.

There is a deploy hook, which should update the ip_info.settings.

Hmm, that's new to me that such a deploy hook exists. Do you have some documentation what's executing that hook?

the update hook, which should enable the Key module.

That's still problematic. It may work in some cases but not in others because the new service declaration already refers to a non-existent service from the key module before it can get enabled.

The problem is that this module introduces a dependency on the key module but there is no upgrade path for existing sites. And that would be pretty difficult anyway, as enabling the required key module fails if the cache needs to be rebuild before then as the ip_info.base service requires a service from the key module, but that doesn't exist.

Also, all existing key config is now lost. There should have been an update hook which turns the existing config into key storage.

Last but not least, the ip_info.settings config stays available. This should be deleted by the update hook.

jurgenhaas → created an issue.

Thanks for reporting this, it seems that setFormFieldAttributes in this action plugin is not taking the no-value into account. We'll look into this.

Is this happening with any form or a specific one? I'm asking because I can't reproduce and it may well be that there is something in the form which contains unexpected data types.

What's required is a more specific way on how this can be reproduced.

The MR fixes the issue.

I'm seeing the same issue and it breaks all our pipelines.

Let's start from the top: the list page that shows all the available config entities of a target module, e.g. /admin/workflow/eca for the ECA module. That list is probably ECA-specific, and the overview page(s) for migrate will be different? If so, that controller should be owned by the target module, not the API.

While being on it, there's probably not only 3 but an infinite number of plugin types. While ECA has event, condition, action, and gateway (not fully, though), Migrate comes with source, process, and destination.

But BPMN_io as an example also has groups, and maybe other interesting components, that are currently hidden from the UI because ECA doesn't need them.

For the target interface, we may want to have just one addPlugin method, which gets one argument that tells about the plugin type, which the target module must have registered before anyway.

Ideally, we have the target modules register the plugin types they provide, and the modelers register the components they support, and the modeler API manages the mapping between the plugins and the UI components. Maybe we can achieve that with some metadata that the target module and modeler provide.

benjifisher → credited jurgenhaas → .

Condition is during comparison and one string is the token for the current path. The other one is your path as a fixed string.

The improved context information would actually be very helpful for ECA as well, not only because the significant number of action plugins that come with it would be of real value to other ecosystems then as well.

@avo webworks, checking for membership and group roles is something that will certainly be implemented. Checking for field values in a group entity is already possible with basic eca_content plugins, because groups are content entities like all others.

If you can implement that in a generic way so that it's potentially interesting for other agencies as well who're using DRD, then it would be amazing to have that contribution as part of this module. I'd be happy to review, feedback on and merge new code.

This is an issue that has been closed for 2 years. In the meantime, a lot has changed, and most of what the conditional_fields module is doing can now be done with the states support for form fields in ECA. But PLEASE: not in closed issues.

Using a patch file from d.o is likewise not recommended, you can not rely on those files being present. The correct way is to download the diff and patch from local files. And you can get that diff from the MR without any issue.

Looks great. And in the processor, where we call $action->access(...$args)->isAllowed(), this can be disabled e.g. per ECA model.

I'm very much in favour of this proposal. Event want to add to the global user switch issue that we found this to be so bad in most cases because most access control features become unusable in that context, and we've banned that feature from all our projects.

My only worry is that separating access from executable checks is not always that obvious. E.g. in the given example with loading an entity, what else should be access controlled if not the view permission on the given entity? And to accomplish that, the entity needs to be loaded, which inherently also checks that the entity even exists. What I'm saying here is that in this case there won't be any access control possible if this will be part of the executable check.

Nevertheless, making access control optional sounds tempting, and I could see a lot of admin ECAs benefitting from that. But that setting should probably not be global or on a per action plugin basis, it should be configurable per ECA model. That's the only way to make them shareable across multiple sites.

OK, I'll mention that in the release notes, hope I won't forget. But we can stil edit them afterwards.

This is great, I remember that I came across that issue before and avoided the context by changing something in the customer project at that point, just because I was too busy to handle it properly.

The MRs are looking good and I will merge them as they are.

Just a question for the release notes: this may be breaking some styling in forms if somebody has some CSS which doesn't expect the container at this point, is that correct?

The first question is, why are you using a webform to create a new user and not the regular user registration process?

But if you do it with webform, you could also add a password field in that form and ask the user to assign themselves a password.

Or is the user created by somebody else, and not the user themselves? Then the user should be notified by email that they can go to the password reset page and request and one-time login-URL with which they can start the process of changing (or initially setting) their password.

And a tip off-topic: if you assign an issue to yourself, it appears that you're working on it already. If you want others to work on it, you should leave the assigned field at unassigned.

Good idea. Please have a look, the MR does it in a similar way as the views related events in the eca_views sub-module.

jurgenhaas → made their first commit to this issue’s fork.

You could use the "Controller found to handle request" event and then check for the current path with a condition.

This sounds great, should be worth a try.

@mxh that sounds interesting, I just wonder where we get the render array from. Does that require an extra event for when the page gets rendered?

Would it otherwise be possible to decorate the \Drupal\Core\Block\Plugin\Block\PageTitleBlock plugin?

That looks great, thank you @scott_euser

Only one (final) consideration: should we move the _gin_user_form_submit into GinSettings as well? Then we have the settings related functionality all in one place.

I've spent some time reviewing the current state of the documentation. It is amazing! Thank you @amber himes matz and @eojthebrave for this excellent draft. I had not seen any description of what we've done in that track for Drupal CMS which comes anywhere close to this document. I can't wait to see this published, especially as I'm presenting in Atlanta about this next week, and referring to it from the slides would be great.

I've left a few comments in the doc, but overall, I'm very excited. Thank you!!!

@gxleano the problem with the implementation in ✨ Add a setting form where Tagify widget can be set as default when a supported field is created Active is that it works for new Drupal installations, whether Drupal CMS or vanilla Drupal. But if an existing site, that already has tagify enabled, will not have any setting for set_default_widget, neither true nor false. That may create PHP warnings when using the return value from $config->get('set_default_widget'), as it's expected to be TRUE or FALSE, but you may get NULL instead.

Therefore, adding an update hook in the module, which will only be executed on existing sites when updating to the latest tagify version, you can make sure to initialize that new config value. New installations of Drupal CMS will not be affected by that, so that they will receive the default value from the recipe, which is fine.

This is looking good to me, the only suggestion is to add an update hook to the next release of the tagify module which sets the default value for existing sites.

Changing status until we gt feedback about further test results.

Closing this due to missing feedback.

Turning this into a feature request, hoping that someone can eventually build something like this.

@handkerchief there is an additional fix in 🐛 OpenSSL" not found Active that will help with this issue on remote sites so that you don't have to re-register remote sites with DRD.

Great finding, thanks @jfauske. That's why it was reported in 🐛 Error: Class "\Drupal\drd_agent\Crypt\Method\OpenSSL" not found Active that they had to re-connect domains with DRD. Fixing the value also remotely was the missing pieve here.

This sounds amazing. As an ECA maintainer I'd be happy to provide help when needed.

@bdunphy that's OK, and I'm sure the logic of that part can be reviewed by maintainers of the scheduler module who know exactly how that works.

I just wonder about 2 things:

$entity->original->publish_on->value ?? NULL

Wouldn't that throw a NULL pointer exception if publish_on didn't exist?

$entity->status->value

Similarly, the status field doesn't need to be called status. I guess the safe way of doing this is to get the property key for status from the entity annotation and then ask for the respective value of that field.

As for an ECA point of view, the plugins looks good to me, I think I mentioned that before.

I looked again as well. The code as such looks good to me. What I can't tell, if the 4 conditions that are chained together is really what tells us if the given entity got published or unpublished by scheduler. If it does, then the rest is OK, except for the failing PHPCS test.

Is there a way to have ECA warn you when importing that some elements (views, fields, etc.) will be overwritten?

No, but that's not required. You can export the config before importing something new and restore to that afterwards. Or, in case of ECA testing in support, just spin up a fresh Drupal site, that's a matter of 2 minutes and no risk for anything else.

When you're saying you load an entity and just got an ID, I wonder how you can know that. Are you relying on the output of a token in a message or something similar? That doesn't tell you much. If you e.g. have an entity and print that in a message, it depends on the entity implementation on what it returns as a string representation of that entity. That can simply be the ID of the entity. But that doesn't mean that the token contains just the entity. In fact, if you use the load entity action, you either get an entity or nothing. But you never get an ID.

If you're uncertain about what values you have and what steps your ECA model takes when you execute it, you can only rely on the debug techniques described in the ECA guide. Printing messages on screen doesn't help ion such scenarios.

Using a view is needed when you want to receive a list of entities.

Load entity is needed when you want to receive 1 entity.

And when you use "Trigger custom event", that triggers a custom event for each of the entities in a list. If you use that action without an event that receives that trigger, this action just does absolutely nothing.

And when it comes to sharing information, a picture of a model looks nice, but it doesn't contain the important information. And don't worry, I'm used to work with ECA models received from users, I won't load that into my production website ;-)

Screenshots or PDFs of models is telling only a very little fraction of what's going on. Please export models in an archive and upload that.

But not for this one. I've seen enough from the PDF that you're calling a custom event right after the view. But where is that custom event? Triggering a custom event is supposed to be calling a custom event for each element in a list (the view result in this case), and then do something for each of them.

I was actually wondering, when we implemented this a few years ago, whether this issue would ever come up. Now, it actually happened, but I'm not sure if we can address this. The whole notification template system is built around the assumption that a notification has a subject and a body. That's what most channels also require, the most popular example being an email notification.

I don't think we have a way to provide a generic solution that wouldn't even have that common ground. Unless, there is something I'm missing, it looks like this module is not for sites that are restricted that much and would allow for a body field.

I describe the architecture once again with my own words, maybe that makes it easier to understand:

• There should be fine-grained controls over which of the theme settings are allowed to be changed by the user and which aren't. Difference to what we have today: there is only one switch that either allows users to override theme settings or not, i.e. all or nothing.
• This part should be implemented in Gin.
• Following my comment #6 this needs some attention as there could always be a user setting around which isn't allowed to be made any longer, so then the global theme setting has to be applied.
• Getting the current values for each setting and getting the setting if a setting field is available to the current user or not, both should be implemented in a service, and all code in Gin needs to call that service to get the values and whether the user can alter the setting.
• For Gin to allow this fine-grained control, it needs extra settings where the admin can decide which settings is accessible by users. This is done without permissions because Gin should work even without the gin_permission module being installed. But those settings are only available, if the gin_permission module is not available, otherwise that module takes over.
• The second step is then to make this permission based in the gin_permission module. Here is where the service comes in, that I mentioned above: in this central place, every time Gin needs to know if the user has access to override a setting, that service calls an alter hook with which the gin_permission module can override the default Gin decision with its own permission-based algorithm.

That way, Gin can work on its own just with settings on which setting can be overridden by users. And the optional gin_permission module can hook into that decision and overrule this settings-based decision by a permission-based decision.

Re-opening the issue as it still needs some work to be done.

I have reviewed the code and left some comments as that needs some more work.

This is now pushed to DRD and DRD Agent, both in their latest dev release. Looking for another test.

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.

This sounds similar to this one: 🐛 Error: Class "\Drupal\drd_agent\Crypt\Method\OpenSSL" not found Active

Thanks, that back port makes sense, of course.

jurgenhaas → made their first commit to this issue’s fork.

Getting close. Two more comments in the MR and a test is also required for this.