🇩🇪Germany @jurgenhaas

This looks more like the field "Cache tags" doesn't support tokens.

The Views: Set filter value action works together with the Views: Pre Build event.

One use case, where we're using it, is a cron run that should send email notifications based on a view result. So, after the cron event, we execute a view with ECA and have that views pre-build event followed by the views set filter value action.

The only shortcoming is that the event can only subscribe to a specific view, but not to a specific display within that view. That's something we should probably add as an optional configuration to that and other events too.

Token is certainly not a dependency of ECA, and never will. It's by design that ECA only comes with Drupal core as a dependency, nothing else.

The \Drupal\eca\Token\ContribToken class, which extends the contrib token class, is only used once in \Drupal\eca\EcaServiceProvider::alter and there it only loads that class after testing class_exists('Drupal\token\Token'). In other words, if that class doesn't exist (i.e. if token is disabled), that the contrib token class from ECA won't be loaded.

The real problem you're facing after disabling the token module, is a cache issue. And due to the Symfony mechanism on how to rebuild the container cache, this is not that simple to resolve, as a drush cr will also fail. What needs to be done in that case, is manually deleting all caches (in db and elsewhere like Redis, if used). Then, the drush cr will work as expected and the site is up and running again.

Thanks a lot @liam morland for reporting and fixing this. I really wonder why this is happening only at your end and not everywhere.

However, the MR makes senseand will be merged and back ported.

@phenaproxima MR!213 contains the switch to tagged releases for ECA, bpmn_io and Klaro.

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

Turns out, the model above doesn't contain any more elements, so the conversion does exactly what's possible and we merge this now.

Any potential follow-up issues can then be addressed in separate issues.

Unless it were made clear that consenting to "Always" implies the consent for the Klaro! cookie which stores that consent.

jurgenhaas → created an issue.

Hmm, that MR seems to make an assumption, that there is a path prefix with the langcode. I think, that's not always the case. And there are probably even more ways to alter the routing.

Isn't it possible to use the Url service from Drupal to receive the correct path?

@phenaproxima I've provided some more explanation in the thread to the MR.

In other words, if you have e.g. 7 conditions and you want to know if either of the 5 first ones is TRUE, you have to choices:

Option 1: OR condition like 1 or 2 or 3 or 4 or 5

Option 2: AND condition like not 6 and not 7

It always depends, which option is less coding.

I'd say yes. Let's hope this doesn't come with unintended side effects.

Thank you @lammensj for all your hard work on this. It looks really great and I've also fixed the remaining PHPCS and PHPStan tests. I don't think we need any further tests at this point.

But I found one odd ECA model, that causes an issue: https://ecaguide.org/library/simple/combined_conditions/

The problem seems to be that there are event objects that have no template associated to them. Do you think that could be fixed?

Thanks @jan kellermann, both approaches do work: turning off twig debug solves it but also the MR!14. It still feels a bit uneasy, it's really strange why DOMDocument is causing such issues.

There is some extra metric at our hands that could be taken into account in addition to usage statistics: lines of code, test coverage, eco system, etc.

Chances are, that we get much closer to the "high value" modules by taking those into account as well.

jurgenhaas → created an issue.

I have to admit that I'm a layman when it comes to legal topics. But following the above discussion seems to be boiling down to a simple question:

Is storing a user preference for, e.g. dark or light mode, technically necessary or not?

I can follow the argument that it is technically necessary because there is no other way to follow the user's preference.

I've rebased MR !5758 since the ckeditor5_theme() got removed from Drupal 11.x but is required for the inline insertion here.

jurgenhaas → created an issue.

jurgenhaas → created an issue.

Would be great if this could be done as we're planning to use this module, but require it to be covered by the security advisory policy.

jurgenhaas → created an issue.

Any plans on getting a D11 release tagged anytime soon?

@phenaproxima since I can't commit to the wiki (permission issue), I've added the ADR file to this MR. Can you please copy that over to the wiki for me?

The scope of tokens is limited to the flow (event and successors) in which they have been defined, see https://ecaguide.org/eca/concepts/tokens/#scope-of-tokens

If you require some tokens also in a custom event, you need to explicitly forward them by providing their name in the Tokens to forward field of the trigger custom event action.

We've further investigated today and found out that we selected the node type "page" with the teaser display mode for rendering the notification content. Unfortunately, that teaser was configured to have a field group with media, title, shortened body and a bit more, and the field group also gets a link so that the whole group can be clicked.

Of course, that's a stupid setup for a notification rendering and we haven't been clever in setting it up this way.

What we should do for this issue, though, is to provide a dedicated node display mode for notifications only. That should only contain title and body, and nothing else. Then, the rendering should work without any issue. As such, that won't solve the underlying issue, that links to the node would still fail, but not using such links in the first place is what makes sense, and that way the problem is avoided.

The new display mode with its basic configuration should become part of the PF package so that it can be used as a quick start. For that, the PF default config should make use of those elements OOTB.

Thanks for testing.

Fixed the remaining thread. There is now a last test failing in the starter recipe which doesn't find the CSS selector nav > h2:contains("Footer") + ul. Not sure if this is related, since that footer menu should be present. So probably unrelated?

As it's no longer NW, I guess, I'm setting this to RTBC according to @pameela's comment.

I've remove the test for the imprint link. There are now some failing tests that all relate to a missing pathauto property. I'm sure that's unrelated to this MR here. Setting back to NR.

Schema issues have been fixed upstream, so the tests are now green again.

I've just tested the issue fork and this works and looks fine to me. The only thing I noticed was a composer error during drush site:install because of missing module menu_link_attributes (installed it manually then to test).

That's strange. It is contained in the require section of the composer.json and listed in the recipe.yml for installation. Have you run a composer update after checking out the MR?

Does anyone know why "Always" is not an option in the first time?

I remember there has been a discussion about that in the Klaro issue queue somewhere. But I can't find it any more. So, as this is not an issue about the way this is being integrated, but an issue in Klaro itself, I suggest to open an issue there so that it can be addressed upstream.

jurgenhaas → created an issue.

This is interesting since that code is in there since 27.12.2015 - almost 9 years ;-)

But wait, we may be hit by the fact that once has been removed from Drupal core, or replaced by a different library. And we just missed to replace that here as well. We can't just remove it as we only want to process that code once, and not every time that Drupal executes behaviors.

Please give the MR a try and let us know if that fixes it.

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

This is not a bug, it works as designed. Using the create new entity action creates an entity without saving it to the database. Before saving, you can use the set entity field value action to set the ID of the parent entity, and you probably want to set other field values as well. Once all field values are set, you can then save the new comment entity.

Nice work, again.

The good news is, the ModellerInterfaces comes with a clone method that is really helpful here. Each modeller will handle the cloning and return a prepared ECA entity. And the Fallback modeller returns the ECA entity as is without cloning. So, that gives you everything you need.

I've committed the necessary modifications to the MR. Please feel free to revert that commit, if you don't like it.

Closing issue as work is being captured in separate issues.

Closing this issue, since our action plan is now in individual issues that are mostly in review already.

This MR is now available for testing by the privacy track team. It only configures the data protection framework in a very basic way.

What it does so far:

• A new tab "All your data" is shown in the user profile where users can request a data export or data removal.
• A task management is available for admins at /admin/config/gdpr/tasks where they can work on those requests. Note: the cron needs to run several times during that process
• The SQL Dump is being configured to anonymize some data
• For the reports, removal and SQL dump only these 4 fields in the users_field_data if being anonymized: name, mail, password and initial email.

This is working fine and if we want to go ahead with this, we can then discuss which other fields needed to be included into the list of sensitive data.

A downside that comes with this setup: the GDPR config section at /admin/config/gdpr comes with 3 items that we don't need/want but that can't be disabled. Those are: Checklist, Summary, Content Links

Another issue may be the name GDPR, as it is misleading for most users, in fact, all users outside the EU.

Still, let's give this a try and then discuss how we want to proceed.

This is now ready for review:

• Imprint is completely removed.
• Privacy Policy is unpublished and comes with a statement that the text needs to be edited before publishing. We may follow up on this to provide some guidance and links on how the site owner should go about filling that node with reasonable content.

But for now, this can be reviewed and improved later, at least we get a clean setup in preparation to Drupal Con Singapore.

Consent management is now configured completely non-intrusive. That means:

• There is no widget visible by default, i.e. the user doesn't notice that consent management is even present on the page.
• Instead, there is a link in the footer menu called "My privacy settings". If clicked, the consent management popup will be opened allowing the user to review and change their consent.
• That said, privacy is still being taken care of, i.e. only required cookies will be allowed and external content (e.g. YouTube videos) will be replaced with their thumbnails until the user gives consent, that the external content should be loaded and displayed.

This is setting all Drupal CMS sites into an ideal privacy compliance state while most users on most sites will never ever get distracted by any of this. I'm sure this will create even higher acceptance out there in the world.

Note: there is one final test failure with a config schema issue which is being addressed at 🐛 Redundant library.notice_as_modal still in default config Active .

jurgenhaas → created an issue.

This is still looking good. I haven't tested all scenarios, but silent and Notice dialog do work as expected.

Thank you so much, this is huge!

jurgenhaas → created an issue.

This is awesome. It can now be configured as if no consent management is available at all, and the site still remains compliant. Wonderful.

Can I suggest to add a link to the https://www.drupal.org/project/menu_link_attributes → module in the documentation as well? That would be required for users who want to create a menu link, and not a link field.

This should be possible with a little tweak of core. It already gathers the host's default timezone in \Drupal\Core\Installer\Form\SiteConfigureForm::buildForm:

    $default_timezone = $this->config('system.date')->get('timezone.default') ?: @date_default_timezone_get();
    $form['regional_settings']['date_default_timezone'] = [
      '#type' => 'select',
      '#title' => $this->t('Default time zone'),
      '#default_value' => $default_timezone,
      '#options' => TimeZoneFormHelper::getOptionsListByRegion(),
      '#weight' => 5,
      '#attributes' => ['class' => ['timezone-detect']],
      '#access' => empty($install_state['config_install_path']),
    ];

That assumes that the correct timezone is set on the host, but I mean that's what we should expect. If not even that is configured correctly, there's hardly anything one can do.

Now, this approach in core only works if the config form is opened and saved.

This should instead be done during installation. Then we should get the correct time zone for "most" new installations.

I can see those concerns and wonder if we should provide separate recipes instead that would be discoverable through Project Browser with labels that indicate who's gonna need what.

Normally I would expect contact details to appear in a block in the footer. Can we not just ship something like that?

That's probably something for the base recipe, not necessarily a privacy topic, if approached that way. And it would require some mechanics to capture that information that should be displayed.

I've cleaned-up the MR with these steps:

• The methods setTitle and getTitle can not be used as they don't exist in EntityInterface. Instead, let's forward the optionally rewritten title as an argument.
• Remove the unused $fields variable.
• Make sure we have a fieldable entity before calling hasField
• Replace method_exists as this would fail if title is either a string or NULL. This part is only required if the advanced render returns Markup, so let's test for that.

This is still working OK, except for the preview. To my surprise, the preview gets rendered without having a render context at that point. It does have a render context, though, when rendered on the page then.

There have been issues about this before, the closest is #2564475: LogicException: Render context is empty in views ui preview → but it got closed as not reproducible. @mandclu as we now have a reproducible case, should we try and report this upstream and see if core can either fix this or tell us what we need to change?

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

OK, I've removed the label change entirely so that we stick with the original label from core.

@phenaproxima I've also rebased the MR to include your changes from yesterday. Leaving this in your hands for the tests now.

jurgenhaas → created an issue. See original summary → .

jurgenhaas → created an issue.

jurgenhaas → created an issue.

jurgenhaas → created an issue.

Thanks everybody for your feedback. We've discussed all this in our today's weekly meeting in the team and suggest the following steps forward:

• Less is more: we should provide some default content but that should not make any impression of being usable in production. Just the opposite, make it so "ugly" that nobody can ever assume this would be good to go.
• As we have different sets of required content, we should provide almost empty nodes for "Privacy Policy", "Imprint" and "Coookie Policy", but only publish the first one and keep the other 2 unpublished. That way, the site owner can enable them when required in their context.
• Use tours to guide the user through the steps we recommend them to take for each of the 3 documents.

How does that sound?

Hey @pameeela regarding your last commit for the checkbox label:

The original label from Drupal core is "Notify user of new account". As this is so close, I'd suggest to not alter the label at all, because then also translations from Drupal core will continue to apply.

In any event, just changing the label in the eca config wouldn't last long, we would also have to update the eca model. The former is derived from the latter.

But I hold off on this waiting for your choice whether we should just leave the label as is. I can then make that change.

Thanks @grimreaper for the heads-up, I've now scanned the whole module and not only found the other info files that needed updating, but also a few deprecations where the used functions don't exist in D11 any longer.

I've fixed all that, but that now requires a decision as the next release can no longer support older Drupal 10 or even Drupal 9 sites. My recommendation is to support ^10.3 || ^11, as most modules are doing that successfully. And everything before Drupal 10.3 is not supported any longer. If any user wanted to stay on any of those core versions, they would have to continue with an older version of entity_share.

This looks nice now, thank you.

Amazing, thanks for addressing and fixing this. And yes, it's the best choice not to get into token handling in tamper ;-)

As for the regex edge case, I don't think it's worth introducing a new type. This is just a scenario where a regex string could easily look like it contains a token syntax, where it isn't. And that would normally remove that part from the regex string, if that token didn't exist.

Supporting array values was pretty simple. We should now also support token replacement right before passing the data to the plugin config.

This is now working OK, after I had to resolve another eca_tamper issue at 🐛 Properly handle arrays in tamper plugin config Active to properly handle the array in the config values.

There is only 1 thing that we can't support with this approach. Sorry, I forgot to mention that before: if the configuration of the plugin is done by using variables (token in the case of ECA), they will only be known at runtime, not when configuring the model. That can not be done now, because the processing of the word list happens during config, not during runtime.

Example, if the word list were defined like this:

[mywordlist]

The value behind this token could be a string or an array, but only known during runtime.

Another example:

myfirstword
[myplaceholder]
mythirdword
etc

That could be the configured string for WORD.

We could probably resolve that by replacing tokens at that point and then update the plugin config. So, maybe not an issue for here. Just left the details so that you can see where we're coming from.

jurgenhaas → created an issue.

@pameeela, we can't tell for sure. But what the model firs is similar to any hook implemented by any of the installed modules. Nothing special, though.

In this case, we just alter the weights, a label and state conditions to some fields. Worst case, that should create an unexpected field order and label, but remain unnoticed. Wouldn't classify that as breaking a site, so the risk seems low.

Sure, if converting back is a no-brainer, then I would also avoid redundant data storage.

And no, there is no need for a processConfigValue method any longer if everything is handled this way. I'd like to give this another test if you already have the build form adjusted.

It feels like we shouldn't spend more time in trying to capture the ambiguous function declaration of that encode plugin from the tamper module. We went back and forth with trying to be smart about data types where that method just wants either string or array, depending on settings, but doesn't provide any means to validate the provided data. There has been an issue where it was suggested to break that plugin apart and be very specific about data types.

Hmm, if that's the plan, then the build form function needs some enhancement. Because that form is meant to edit die configuration and it uses the word settings. That would then always be an empty string. Converting the list back to words isn't simple either.

So, for that reason, why not keeping the original word settings. Is there any downside to that?

This looks amazing and is a great new feature.

WRT preprocessing, could that be made a little more generic? I.e. checking if the media entity has a thumbnail entity. That way, we wouldn't have to hard-code the bundle name remote_video as that name could be different on other installations.

@megachriz thanks a lot, I've looked into this and ECA is calling the tamper plugin's submit method before storing the configuration into the ECA config entity. So, this way we're able to receive the correct data. And as the keyword filter plugin keeps both, the words as a string and the word_list as a list, editing the configuration later should still work based on the original string that contains all the words.

Oh, and another note that I've updated the bpmn_io dependency to 2.1.x as this branch comes with some exciting new features and we'll publish a stable release together with ECA 2.1 before the end of November.

Great you like it. I've remove the empty password validation part and it's ready for review again.

@phenaproxima the test is failing with a 500 return code when the redirect test goes to /admin, but I can't reproduce that locally, neither with Drupal CMS nor with standard profile. Also, what's the scope for tests here?

Thanks for reporting this. I think, we should keep the functionality and just fix the labels, as they are certainly incorrect right now. The reason for this, it would then be in line with what the cache interface of Drupal really does. And that's what ECA utilizes in the background. For details see documentation of \Drupal\Core\Cache\CacheBackendInterface::set.

And yes, we should add token support for the expire field but also for the tags field.

If you wanted to start with an issue fork and get your code changes into it, we can easily take that as a starting point and improve it together.

But feature requests need to go into the 2.1.x branch.

Yes, that's exactly what we're looking for. I've looked into it and there are a few things that need to be done:

• Security advisory policy
• Drupal 11 support
• ECA 2.0 support
• Improve discoverability of the module, or merge it with ECA core

I've pinged the maintainer @zipme_hkt to see if they want to discuss this with us.

jurgenhaas → created an issue.

1. Do you prefer "convert"? Might capture the intention better... I guess it's a matter of semantics

That sounds good, yes.

3. Now that were working towards multiple modellers being able to alter the same ECA-config, the question rises if the admin UI should be altered/optimised? Because you would have multiple rows, concerning the same ECA-config, but different modellers which aren't kept in sync.

We probably don't need that. I see 2 different use cases:

The first one is for models that are not created by bpmn_io and not by fallback, they should be converted and saved under a new ID. That way, the original modeller continues ownership and maintenance of its own model and bpmn_io gets a copy of it under it's own regime.

The second one is for models that have been created by fallback, i.e. not really a modeller that has an intent for continued maintenance. For those, we only need to convert, change the modeller to bpmn_io and that model is now owned by bpmn_io.

That solution assumes that if the AI agent creates a model it has no intend to ever update that model, does it?

Wow, it works!!!

Just a small issue: it doesn't change the modeller ID when saving the updated ECA model, and it remains at the path /admin/config/workflow/eca/eca_lib_0029/clone/bpmn-io which will continue to use that new route which triggers an upstream conversion again.

The expected behavior should be that it updates the modeller ID and redirects to /admin/config/workflow/eca/eca_lib_0029/edit after the initial save that's following the auto-layout.

And an additional idea, but then for a separate issue: the auto-layout feature is probably worth exposing as a function in the canvas in general. What do you think?

I've reviewed the code and this is impressive! Great work @lammensj, very exciting. I will give this a try in a minute but wanted to leave to small but general thoughts:

• Is "clone" the correct key for what's happening here?
• This keeps the ID of the original model, which is from a different modeller. That means it overrides a config entity of unknown source. For the context of AI-generated ECA models, that's exactly what we want, but maybe not for others?

That brought me to the following idea: we do have a fallback modeller plugin. How about a convention that AI tools should generate models with that fallback ID and those models will be converted and overridden. All others will really be cloned so that we keep the original.

Again, thank you so much. This won't take long to get merged. And the community will love it.

This can now create the following types of random values:

• Bytes
• Image
• Integer
• Machine name (unique)
• Machine name
• Name (unique)
• Name
• Paragraphs
• Password
• Sentences (capitalize all words)
• Sentences
• String (unique)
• String
• Word

This is now ready for a first review. It works as described in ✨ Allow per-site configuration of 'Notify user of new account' default setting Active with one extra that was unnoticed so far: to make this work, the password field can no longer be required; otherwise the form couldn't be submitted as the random password will be set after submission and will be empty before then.

To resolve this, the ECA model adds a form validation for an empty password in case that the notification is turned off. Because then we expect a password to be set. So, we would now get a traditional form validation error in this scenario, which is less user-friendly than the inline form validation that we have with the required password field.

An alternative solution would be to accept an empty password, since nobody could log in to such an account without using a password reset.

Please have a look how this works for you. Once we have the final version of this, writing tests should be done afterwards.

jurgenhaas → created an issue.

I'm on a slow come back and started working on this. The user register form looks already like the animated gif in the related core issue. I'll keep working on the submission and creating a random password next.

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