🇩🇪Germany @jurgenhaas

Logs are not present

There must be something somewhere. Either in Drupal's watchdog or in the PHP logs. It's hard to watch the video and getting an idea what's going on. If you really can't find any logs anywhere, we then need step by step instructions on how to reproduce this behaviour on a fresh Drupal installation.

Why would anyone want to edit requirements? Those are declared by Drupal core or by contrib modules. And they shouldn't be modified, as the declaring instance has a reason to define them.

Or am I missing something? What's the use case? Can you explore on this a bit, please.

OK, please give this MR a try, it should be fixing it.

The reason is that with the ajax request the form ID changes and the Gin javascript is correctly updating the new form ID for the action buttons and adds a new event listener to them. However, the previous event listener remains in place, and that uses the old form ID.

The solution can be one of these:

• Remove the old event listener before adding the new one
• Let the old ones run as well but prevent the error from happening

Will push an MR in a minute.

Thanks everyone.

Setting this to fixed in order to contributors earning issue credits.

This sounds like being related to 🐛 ECA Form breaks complex IEF widget Active . Can you provide any more details from the logs please?

Merged and back ported to 2.0.x as well.

Thank you so much for all the careful testing and the great feedback, really much appreciated.

Good catch. It looks like the access and execute callbacks need to be overridden in this action plugin. By doing so, I've also added a loop to go through all bins to invalidate all their tags, if they're empty.

My only concern here is one of performance. If token substitution happens, but yields an empty string, does that mean that this kicks in?

When empty, then the whole cache is being invalidated.

For larger sites, full cache invalidations can lead to some pretty significant performance degradation, and in some cases denial of service if the ECA workflow runs often enough.

That's correct, but that's by design, I guess. Just like with other means that allow you to flush all caches. There are options in the UI as well as e.g. drush that deliberately provide that feature. So does ECA.

With great power comes responsibility, doesn't it. ECA models should be created with care, so that flushing all caches doesn't happen unintentionally. I would expect that enough eyeballs are looking at such ECA models, like they would review custom code which could cause the same issue, especially on those large sites, where this really makes a difference.

@luke.leber thank you so much for the detailed testing.

Now wondering, if we should merge this bug fix or not. The fact that ECA could wipe all caches has been there from day 1.

Note to self, here is a snippet from @webflo on how he addressed this before: https://gist.github.com/webflo/67a380250486f8ab2ccacc73efe21406

I've started a new module eca_group and in there a meta issue 🌱 [Meta] Feature collection Active where we should collect features to implement. Closing this one here since this will be done in the other module.

jurgenhaas → created an issue.

Eventually managed to get this done. We now show 3 events per model max, followed by an ellipsis to indicate that there are more. When clicking on them, the remaining events will be shown as well.

That way, the meanwhile implemented inline search on events continues to work and also finds those that are not displayed.

Setting to fixed as I was able to reproduce the problem and can confirm that the referenced MR fixes it.

Setting this to RTBC according to comments in the MR.

In the case of "Tamper: Get string length", a given field is specified between [ ] brackets and it works properly.

That field asks for a string, and the config field supports tokens. So, if you provide the value via a token, then of course you need to provide token syntax. Just as an example, the config could also look like `This is the [text] that should be measured.`. This is a string that contains a token and that would be replaced before calculating the string length.

You claim that in the example model you sent, if you specify the given field without [ ], then your system correctly recognizes the "empty" and "not empty" states?

I did not send an example model. And I did not say that you should just remove the brackets either. What I said was that you need to provide the field name in the config for that action plugin. The field name is `field_szoveg1` in you example model. That name is what that action plugin requires.

If you can't get that to work, please provide another exported model that we can review what's wrong.

What's the status on this?

What's the status on this?

What's the status on this?

What's the status on this one?

Ping @grienauer

We've changed the positioning accordingly and are pretty happy with it. Thanks @ressa

I've added an extra sentence to the description of that token.

Disabled the import form if the config module is not enabled.

Hi @ken hawkins any chance of getting back to this?

I've rebased the MR to the latest dev branch. @VladimirAus maybe this is an issue to be addressed in Singapor?

I've addressed all open threads and also moved the re-throw below the logging to address the final part of #7

This needs a review now, and hopefully we can get this in to the soon to be published 2.1.0 release.

Closing for now. Feel free to re-open when needed.

This is out of scope for ECA, it's more about CIM and staging.

It's unlikely that we're getting back to this, therefore closing it.

Contrib integration is a permanent process that works really well. We don't need to push for that explicitly anymore.

It seems very unlikely that we get to this general approach at any time, especially because we now have almost 1.000 plugins that would have to be covered by this generic approach.

It's more likely that individual plugin texts will get reported and improved in separate issues.

Upstream conversion from a pure ECA model to one that can be edited by the BPMN.io modeller has been implemented in the bpmn_io module.

@eswiderski, @luke.leber any chance for a review on this? I'd like to merge this before we publish the stable 2.1.0 release soon.

Solved this by wrapping the render array only if the specific view gets post rendered, not before. That way, ECA models can use this event on all views except entity references. But the latter views won't be affected.

jurgenhaas → created an issue.

how/where I can see your comments in context

When you're logged in, both on d.o and on GitLab, you can click on the MR above and you'll see all the comments within the code diff. Direct link to that is https://git.drupalcode.org/project/eca/-/merge_requests/457

As for 1), you'll find more list related actions in the eca_base submodule as well, that's why I thought it should belong there.

As for 2), this is not a requirement, I leave that totally up to your decision.

Seriously, when you export from ECA, you get a tar.gz file.

What do you mean you tried without brackets? Can you let is know what exactly you out into the field "field name"?

This is not about tokens, it really asks for a field name.

How did you export that model? What you should get when you use ECA's export is a *.tar.gz file, but not just a gz file.

Anyway, I've looked into the model and the error is in the configuration of the field_value_empty condition. You're using [node:field_szoveg1] as the field name, but that will replace that token with the value of that field. What you want to use in that configuration field is just field_szoveg1 because that's the field name.

Thanks a lot @mparker17 for reporting, analysing and fixing this. I'm not sure either why this is happening, but your approach doesn't hurt at all and makes that code more strict, which is always great.

The only caveat is that bug fixes should always go against the latest dev release, 2.1.x in this case. And from there, they will be back ported. So, I'm committing your fix against that release and close the MR instead. But everything else ist just great.

Thanks again for your contribution, much appreciated.

It looks like this for me when I'm using your custom code from the IS:

I'd say, if you're adding custom throbber to any element like a submit button you also need to deal with the look and feel of it. A suggestion has been made by @mostepaniukvm in #4 and that works perfect with a throbber in the center of the screen.

All throbbers that I was able to find that come with Drupal core and/or Gin are looking just as they are expected. An example is the cardinality number in the field UI.

I tend to set this to "Works as designed" if nobody objects?

Adding another related issue.

I was able to reproduce this. The problem is that with the scenario from IS the page has 2 form buttons with the [data-drupal-selector="edit-submit"] and that confuses the Gin script. I've resolved this by making the click selector more explicit by also including the data-drupal-selector of the form to the button that we're intending to click.

This got committed to the MR!536 in the related issue. Please give that another try and let us know here, if that fixes the issue for you. You can then set this issue to RTBC.

jurgenhaas → created an issue.

This is great work, thank you so much @glottus

Yikes. Sorry for all the cruft above. Fumbling my way through!

No worries at all, you're doing great.

I've left a few remarks in the MR. Nothing big, just some clean-up and wording items. Some of it are just questions, please feel free to decline suggestions in there.

Thanks for testing and feedback @diwakar07. Marking this as fixed and granting issue credits, the fix will be merged with the related issue asap.

Looking more closely, this is most likely an ajax error that cypress sees in the browser console. So, the request to /node/add/page is fine, but there subsequent Ajax request by auto save is causing that 500 error. Most likely a timing issue, therefore random. Hope that helps later when we get to this.

I've reviewed the failing e2e test and it comes from autosave_form when trying to add a basic page as a content editor. Tried to reproduce this locally, but I can't. Is this a temporary failure in the pipeline, maybe?

We have been using that condition successfully before. Could you please provide us with a simple model that demonstrates the problem, ideally without any dependencies other than Drupal core?

I'd say that too many options can be counter productive. Instead, let's use CSS variables, e.g. those from common themes that they use for primary buttons. Then, the look and feel will follow the current theme.

Thanks for your feedback and tests. Yes, the next button is not treated as a primary button. I would defer that to the simple_multistep module to mark such buttons as primary if they want them to show as such.

I'm marking this as fixed and grant issue credits, as the fix will soon be committed from the related issue then.

Thanks everyone for your help.

New release tags for Klaro and for Leaflet are available, so this issue is no longer blocked.

Thanks for your contribution.

No patches, please. Turn this into an issue fork with an MR so that tests can run. Once all tests are green, please set the status to "Needs review"

Sure, but please keep in mind, recipes are a thing. And they make it much easier to provide default content (= menu item) and tailor configuration.

And the recipes from Drupal CMS will be available stand alone soon as well, so everybody will benefit from them, even for a vanilla Drupal site.

Just FYI, the recipe in Drupal CMS does that already. Whether the module itself wants to provide something like that as well, is up to the maintainers.

@glottus thanks for going through this, I hope it is a somewhat fun experience. It looks great so far.

Just a couple of issues with the tests. Would be great if you could fix them and get the tests to green again. After that, you could set the issue status to "Needs review", then we can have a look and merge this asap.

Is there a simple way to achieve this?

As I suggested in #2 you can load a route parameter into a token. That's all you need.

jurgenhaas → created an issue.

That library from #2 is being installed automatically by this module and it is a mirror of the upstream library just to simplify installation. In fact, regular users don't need to do anything other than installing this module, and they're all set. This also works for recipes and within Project Browser.

The option to install your own library as shown in the extra composer file is only required if you want to override the OOTB library with your own.

Hmm, I'm not sure I'd like that change. In fact, I'm really excited that this module does it right by not focusing on Cookies. I find that misleading. Having said that, I know that the European legal industry has done a terrible job and planted that term in everybody's brain. But it's still wrong.

I would expect this module to become very popular very quickly, so that should really be an issue any longer, it will be found or known by everybody. At least that's my hope.

That underlines our plan to eventually join forces with Klaro!

This is great news. Imagine we have all privacy experts of the Drupal community behind one global solution, well maintained.

And sad I never recognized that project before

A lot of us can relate to that. Klaro has been a well hidden champion. Looking at the usage statistics for the module, our decision already makes a difference and we should see a great future, not only for the module but also for all its users.

Please decide if we should enable or disable this service by default - Please decide whether we activate the server by default or not - you know the goals of Drupal CMS best. Then I will merge and tag a new version.

I'd say, not just for Drupal CMS but also for Klaro on its own, that the consent management widget should only expose relevant services to the user. So, the question should be for vanilla Drupal sites, whether that will come with leaflet installed or not. And then, then default config should follow that assumption. I guess, leaflet is not present on most sites, and therefore I'd set the default to disabled. But I might be wrong.

Regardless, for Drupal CMS we know that leaflet is not enabled by default, and would override that config in the recipe to disable the leaflet service. It will then be enabled by the event recipe, which will also enable the leaflet module.

That way, we're flexible and can deliver the best default settings for both contexts.

Thanks @anybody for youre input. We've decided to go with Klaro for Drupal CMS and here is our ADR that explains the decision process: https://git.drupalcode.org/project/drupal_cms/-/wikis/Architecture-Decis...

The other options have been ruled out for reasons also explained in that ADR. In short:

• Cookies module: the external library is not open source and the maintainer was unwilling to talk to us, despite huge effort to get in touch
• EU Cookie Compliance: big module, most users, but growing out of its scope and would have required a major rewrite of the module, according to the maintainers. Instead, they have been happy to change direction and provide a migration path from EU CC to Klaro for all existing users.

This is correct, but it won't look broken. Instead, it explains that a map will be loaded if there user accepts that their personal data will be transferred to an external service. This consent is managed inline and of the user clicks yes, the map will load dynamically.

This is similar his this already works with remote videos.

This is leading edge implementation on how privacy is supposed to be working on the web in general. Drupal will be leading the pack once again. 😀

jurgenhaas → created an issue.

@sprucemoose nice! The ECA Guide is written in markdown and maintained on our GitLab instance at https://gitlab.lakedrops.com/drupal/documentation/eca

We've turned off signup because of spam. If you PM me your email address (maybe on Slack), then I can create an account for you. You could create a fork and write a page somewhere in the structure of the guide. We can still move it elsewhere later on then.

For further details, a chat on Slack (are you there?) is probably more practical.

Just wondering, this service should probably be disabled by default and only enabled, when leaflets are actually in use. Otherwise users see this service in the consent management without reason.

This works like a charm :-) Thank you so much for the real quick turn-around. I'll ping some people to hopefully get the leaflet issue committed soon.

This sounds all great, I wish this could be turned into a documentation page in the ECA Guide or a tutorial, as the information in the issue queue is hard to find. If anyone is up for that task, I'd be happy to guide you through the process.

jurgenhaas → created an issue.

Something to consider, when addressing this to enable the consent management services for GA and GTM, is also the visibility of the consent management widget.

As a default, the privacy track has made that widget invisible, i.e. most users won't notice it, ever. This is suitable for most sites, as users find cookie banners and other widget annoying. For good reasons.

However, for GA and GTM, the priority might be different. If a site wants to track users, it is more interested in getting their consent. Therefore, it might be suitable to enable the widget to show to all users by default on those sites that apply the analytics recipe.

If that's the case, then the recipe should also enable that with this:

config:
  actions:
    klaro.settings:
      simpleConfigUpdate:
        dialog_mode: notice

jurgenhaas → created an issue. See original summary → .

jurgenhaas → created an issue.

When you add [entity:nid] to one of your messages, you can see that its output is empty. So, that token doesn't exist.

But that model has even more issues:

You will get the message "not entity.node.canonical!" more often than you may expect. It will be shown on node pages every second time, because there is also always the request to /history/[NID]/read which doesn't match the route entity.node.canonical and that will result in your first condition to be false, and therefore this creates that message which will then be shown for the next page request of the same user, even if they are on a node page.

Then, your scalar comparison of [entity:nid] and 23915 will return FALSE, even if you negate the condition. This is because you're forcing a numeric comparison. And comparing either an empty value or the value [entity:nid] in a numeric type will always return FALSE, regardless of the negate setting.

@mlncn with the latest changes in 📌 Improve sticky actions implementation Active my testing shows that:

• '#gin_action_item' => TRUE works reliably
• '#button_type' => 'primary' makes any visible button appear like a primary button
• '#button_type' => 'normal' makes any visible button appear as normal, even if it were primary without that setting

Please give that a try and let us know if you can confirm. If so, we can then put that status into the API documentation as you suggested.

The preview button issue is out of scope of this issue. But as @dtfabio already mentioned, that's fixed. His linked issue 🐛 Cannot opt out buttons using #gin_action_item = TRUE Active has been merged into 3.x-dev and there is also work going on in 📌 Improve sticky actions implementation Active which I recommend applying for further testing.

When it comes to the original topic of this issue, this is not necessarily a regression. It's a change in the markup of the action buttons that automatic testing needs to adjust to. We've had such changes in Cypress changes as well.

For the background:

The buttons are still present, but hidden in the form. What the user sees instead are the labels in the sticky top bar. However, the automatic tests in Behat, Cypress or other platforms, doesn't care about the visuals, they look into the markup and realize that the buttons are there but hidden. That gets them to assume that they are not clickable.

In Cypress, this can be resolved by enforcing the submit instruction. That way, Cypress ignores the fact that the button is hidden.

Not sure, what's the equivalent for that in Behat, but there should certainly be one. @eduardo morales alberti is there a way you could find out and let us know? I know, it's unfortunate that you have to update your tests, but these sticky action buttons can only be implemented that way, so we have to ask everyone to update their tests in that context.

I can't reproduce this either. Throbber is visible whenever I expect one to show. @artis.bajars could you please retry? Also, please apply the patch from MR!536 from 📌 Improve sticky actions implementation Active which contains the latest fixes around sticky action buttons.

The code around this issue got refactored in 📌 Improve sticky actions implementation Active . Please give that a try and let us know if you can confirm that it's fixed.

jurgenhaas → changed the visibility of the branch 3486966-ginformafterbuild-does-not to hidden.

@mlncn We're currently working on 📌 Improve sticky actions implementation Active and with the latest version of the MR!536 the javascript error in the IS should be gone. Would you mind giving that a try and letting us know if that's true?

If not, please help us to find out what's still wrong with that, as we need to iron that out before we start looking into form_ids_to_ignore and route_names in the \Drupal\gin\GinContentFormHelper::isContentForm.

I was able to reproduce this and fixed it as part of 📌 Improve sticky actions implementation Active . Please test MR!536 and let us know if that fixes the issue for you as well.

Note: you don't need to create a patch for testing this. You can either check out the branch of that MR or you use https://git.drupalcode.org/project/gin/-/merge_requests/536.diff as your patch. This is the best way for testing, as long as you don't do that in production, as that patch can change over time, until it will be finally merged.

The caching approach is not required for this. Instead, we can use a simple static variable to resolve this issue instead of using drupal_static.

I've fixed that as part of 📌 Improve sticky actions implementation Active and it works with simple_multistep. Please test MR!536 and let us know if that fixes the issue for you as well.

Note: you don't need to create a patch for testing this. You can either check out the branch of that MR or you use https://git.drupalcode.org/project/gin/-/merge_requests/536.diff as your patch. This is the best way for testing, as long as you don't do that in production, as that patch can change over time, until it will be finally merged.

We're aiming to address all the related issues in this single MR with the objective to get them all tested and fixed by this single MR. This makes it easier to test for all the reported issues with the action buttons in context of each other. That should avoid new regressions.