Offenburg
Account created on 19 January 2011, over 13 years ago
#

Merge Requests

Recent comments

🇩🇪Germany mxh Offenburg

So many modules... Can this change be added to the automated D11 compatibility bot?

🇩🇪Germany mxh Offenburg

I'm using Linux Mint 21.3 (based on Ubuntu 22.04) and it shows that this requirement is inconvenient for testing D11 locally.

🇩🇪Germany mxh Offenburg

Obviously section steps to reproduce is missing. Without those steps, I can't help you out. Please provide steps to reprodue, ideally starting from a fresh Drupal core installation using standard or minimal profile.

🇩🇪Germany mxh Offenburg

I'd estimate this one to already have an obvious resolution and mark it as fixed.

It's not good a good thing, just opening new issues where maintainers / contributors are trying to help out and in the end are being left unanswered. It's not healthy for the sustainability of an OS community. Giving feedback is important, also for issues like this.

🇩🇪Germany mxh Offenburg

The description of that action says: "Create a new user without saving it."

🇩🇪Germany mxh Offenburg

I'm sorry I'm not available.

🇩🇪Germany mxh Offenburg

@mxh if you want to continue maintenance of this modeller, the constraint for ECA may be opened up to allow any ECA version, and then it probably also needs some attention for the Drupal 11 release, which is due in July.

Yep, thanks for the note. Definitely it will require at least an update regards supported versions, and possibly some adjustments for being compatible with D11. I'll take care of that when possible.

🇩🇪Germany mxh Offenburg

I've marked the project to be obsolete for now. When someone will take notice and has any objections, please post them here.

Leaving the issue open for any potential discussion. Will close it in a few weeks or months when no one stops by.

🇩🇪Germany mxh Offenburg

How do you want to turn off access checks? It's worth noting that current implementations of the access method of most ECA-related action plugins are not only performing access checks, but also validations, as can be seen in the commits of 📌 Enhance (or add) access control in plugins where required config fields support tokens Active . If we'd want to be able to optionally turn off access checks, I currently see two options:

  1. Refactor all access method implementations, so that the access methods exclusively contain access checks (probably mostly permission or role-based checks) and put any other logic such as validation and appliance checks into separate method(s)
  2. Skip all permission/role-bases access checks within the access method implementations e.g. with wrapped if statements that check whether access checks are enabled/disabled.
🇩🇪Germany mxh Offenburg

Thanks for the report and provided patch. I'm currently looking for an active maintainer for this project and already asked in 📌 Add Drupal Gitlab CI template RTBC . In case someone is interested in taking maintainership, it would be great to reach out.

🇩🇪Germany mxh Offenburg

Thanks for the report and provided MR. Does someone of you want to get maintainer write access to this project?

🇩🇪Germany mxh Offenburg

@osopolar I've added you as a maintainer to the project.

🇩🇪Germany mxh Offenburg

there is a litte typo typo in the filename: tar_.gz

That's on purpose for security reasons. d.o. automatically renames the file endings when uploading archives.

maybe using hook_block_access would be a little bit more flexible, as eca_access can only react on ECA Access events.

Could you please elaborate what you're currently missing?

hook_block_access is a variant of hook_ENTITY_TYPE_access for block config entities. The eca_access module reacts upon hook_entity_access, i.e. it is basically already reacting upon that hook.

🇩🇪Germany mxh Offenburg

It's already possible with eca_access. Attached an example model that denies access on the site branding block used in the Olivero theme.

🇩🇪Germany mxh Offenburg

Probably the easiest fix is to add another option to the build mode, which does nothing on the current renderable array in scope.

🇩🇪Germany mxh Offenburg

I created a PoC in !420 and played around with that for a bit. Not sure whether that's a good solution path. Had to add a recursion level flag and implemented without a hook inside ECA's token service, as hook_tokens are a bit late and might require more implementation logic when using hook_tokens instead. At least it just needs one additional event, then you can use already existing actions of eca_base like "Token: set value". Attached a small example model to demonstrate how it could work.

🇩🇪Germany mxh Offenburg

When it comes to back porting, I'm uncertain how we should treat that one.

It's up to you :) don't know if and how someone is already using that particular feature.

As for the token usage in render action plugins, should I open a new issue to talk about the question if and how we could prevent the output of the render plugin when a token is being used?

Yes I think it makes sense to handle that one separately.

🇩🇪Germany mxh Offenburg

I've added a further step into the getString method to avoid objects within the array that is about to be encoded to Yaml in https://git.drupalcode.org/project/eca/-/merge_requests/419/diffs?commit...

For the raised concern in #11 I've added a render step. However this might be a behavior change that should only be included in 2.0.x, if that should be included anyway.

🇩🇪Germany mxh Offenburg

Thanks for the report and providing a patch. I've seen that current maintainers of 2.0.x are not really active here. @osopolar Do you want to become a maintainer of this project?

What's a bit strange to me is that the URL endpoint seems to not work properly now, but used to work earlier, otherwise this client integration should've not worked from the beginning?

🇩🇪Germany mxh Offenburg

Created !419 where I've added a typed data wrapper for URL objects and added support for it into the DataTransferObject. Another step I've added for any unspecified object that can be cast to a string, so that should raise the compatibility a bit more.

Worth noting that !419 only works for 2.0.x, it needs an extra backport MR for 1.1.x since typed data plugins started supporting PHP attributes starting in 10.3 (#[Data(...)), before that only Doctrine annotations (@DataType(...)) are supported.

🇩🇪Germany mxh Offenburg

Committed the points raised in #2 into !418. However this is not yet thoroughly tested, but hopefully helps for understanding.

🇩🇪Germany mxh Offenburg

The following test needs to be changed back to use [config:name] instead of [site:name] in https://git.drupalcode.org/project/eca/-/commit/24a274939e473c61d30989a1...

Method Drupal\eca\Token\DataProviderInterface::getData needs to return an instance of Drupal\eca\Plugin\DataType\DataTransferObject when it wants ECA to handle non-scalar values for token replacement.

🇩🇪Germany mxh Offenburg

Committed, thanks!

🇩🇪Germany mxh Offenburg

All those are very old update hooks. Are you still running them?

🇩🇪Germany mxh Offenburg

I'm not sure I understand #26.

My comment #26 is too long ago to provide more context at this time. Guess I made that advise up to minimize the potential compatibility breaks, since it seems both methods are detached from each other on runtime, which is probalby not reproducable by static code checks but will pop up once recognized on runtime.

🇩🇪Germany mxh Offenburg

Patch of #2 makes sense. Currently there is an implementation as follows within Drupal\payment\Plugin\Action\UnsetLineItem:

<?php
/**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static($configuration, $plugin_id, $plugin_definition, $container->get('string_translation'));
  }
?>

hovever the plugin currently does not implement the according interface, which prevents third party modules like ECA to recognize the instantiation method, leading to the problem reported here.

🇩🇪Germany mxh Offenburg

Thanks for the report and the provided patch, will take a look when time permits. Do you want maintainer access to this module?

🇩🇪Germany mxh Offenburg

Might be worth first clarifying 📌 Consider dropping maintenance support for this module Active before putting any more effort into this issue.

🇩🇪Germany mxh Offenburg

Since the breakpointsLoader.js script does not contain any reference to the Drupal object, I currently don't know whether this is the right fix. Please provide some steps to reproduce if possible, or any sort of details that may help to understand why this error occurs.

https://git.drupalcode.org/project/theme_breakpoints_js/-/blob/8.x-1.x/j...

🇩🇪Germany mxh Offenburg

You might already know that since this issue is not yet fixed, but anyway I've seen that currently most of the tagged form fields do not show the additional descriptions that are implemented in \Drupal\eca\Plugin\ECA\PluginFormTrait::updateConfigurationForm.

I've tried the condition plugin eca_current_user_id and action plugin eca_token_set_value that both have one of the newly introduced tags, but they're not showing the additional description:

Besides that, the currently implemented loop in \Drupal\eca\Plugin\ECA\PluginFormTrait::updateConfigurationForm is problematic:

<?php
protected function updateConfigurationForm(array $form): array {
    $containsTokenReplacement = FALSE;
    foreach ($form as &$value) {
      if (!empty($value['#eca_token_replacement'])) {
        //...
      }
    }
    unset($value);
?>

The problem above is that the foreach statement iterates through a renderable array, which may or may not contain arrays as $value. One proper way to iterate through its children is the following:

<?php
foreach (\Drupal\Core\Render\Element::children($form) as $child_key) {
  $value = &$form[$child_key];
  // ...       
}
unset($value);
?>

That loop needs to get fixed, so that we are able to pass through renderable arrays that already contain render information. Otherwise we may get fatal errors as !empty($value['#eca_token_replacement']) currently always tries to access $value as an array.

🇩🇪Germany mxh Offenburg

The problem here is that you're POSTing data via JSON API, not via a form. The name pattern is currently only automatically applied when submitting via entity forms (or programmatically). When POSTing data via JSON API, the entity that is supposed to be saved will be validated before, and since you're not posting a name property, the validation will fail.

So the missing part here is that the automatic name pattern is not being applied when trying to POST data via JSON API. I don't know whether that's possible. If it's possible, contributions are welcome.

Until then, the only way is to apply the name pattern yourself when POSTing via JSON API.

🇩🇪Germany mxh Offenburg

One possible way I currently see is to add a new value for the "Method" option called "Set and enforce clear previous value" set:force_clear where force_clear empties the arrays before set is being applied:

// ...
switch ($method_setting) {

          case 'force_clear':
            $keep = $existing = $current_values = [];
            break;

          case 'clear':
// ...
🇩🇪Germany mxh Offenburg

Just noticed there is an "admin_permission" annotation available for simple access checks on entities without complex access control. The annotation is also available on the requested "message" entity type: https://git.drupalcode.org/project/message/-/blob/8.x-1.x/src/Entity/Mes...

We probably should use the defined admin_permission to check for access, in case we have no access control handler available.

Also see documentation of Drupal\Core\Entity\EntityTypeInterface::getAdminPermission:

/**
   * Gets the name of the default administrative permission.
   *
   * The default \Drupal\Core\Entity\EntityAccessControlHandler class checks this
   * permission for all operations in its checkAccess() method. Entities with
   * more complex permissions can extend this class to do their own access
   * checks.
   *
   * @return string|bool
   */
  public function getAdminPermission();
🇩🇪Germany mxh Offenburg

As mentioned in #3, the following compromise could work out for all: I'd suggest to at least make a "minimal" access check then when no access handler is available: Check whether current user has an admin role and if so grant access, otherwise deny it.

I think with that approach we at least did the best we can in such case (esp. for an extreme case as mentioned in #5).

Another approach could be to introduce a new option for skipping access checks, something like mentioned here: https://www.drupal.org/project/eca/issues/3348425#comment-15037117 📌 Evaluate consistency of access checks when using fields and loading referenced entities Active

🇩🇪Germany mxh Offenburg

However, if there is a content entity class which has neither any UI component nor any access control handler, it seems safe to assume that those entities are there to be CRUD programmatically only.

I don't know, as long as there's no documentation source or anything like that, this seems too much of a subjective assumption. My assumption is: when an entity type is supposed to be always allowed to be accessed, its according access handler would just always grant access. But when the access handler is missing, I'd say that it is simply not defined, so we can't tell whether access should be granted or not.

Let's put in a worst case scenario: A developer who introduced a bug that accidentally removes the access handler from an entity type definition. Then ECA would (wrongly) grant access to that sort of entity. Maybe this is a bit of extreme edge case, but it's not impossible.

As ECA is not consistent regarding access checks (#3348425) this argument can be seen one way or the other, and this is just my opinion. You finally decide which direction ECA should follow among access checks. Finally at least it should be a consistent behavior that ECA users can trust on.

🇩🇪Germany mxh Offenburg

Great, nice work! Took a look into the MR and haven't found anyhing wrong. I couldn't check every changed imlementation in detail, as this is a huge change. But for that let's hope our existing tests are sufficiently covering.

Added a small commit that removes the event stack from the execution event subscriber, as it's not needed anymore.

🇩🇪Germany mxh Offenburg

Setting to NW for the (probably last) required to do, which is the removal of Drupal\eca\Event\ConditionalApplianceInterface.

As mentioned in #15 we have some event subscribers, that make use of interfaces. For example Drupal\eca\EventSubscriber\EcaExecutionEntitySubscriber checks for Drupal\eca\Event\EntityEventInterface. I think for being more consistent in this refactoring manner, it would also make more sense to move that logic into the according plugins instead, and then finally removing that used interface (in this example Drupal\eca\Event\EntityEventInterface). IMO this is not a must-have, especially because that type of interface is also being used elsewhere in other implementations. Similar goes for other such interfaces like Drupal\eca\Event\FormEventInterface and Drupal\eca\Event\RenderEventInterface. Just mentioning here for sake of completeness.

🇩🇪Germany mxh Offenburg

That's great! Took a quick look and found following missing to do's:

  • Since we want to drop double support, we need to entirely remove the interface Drupal\eca\Event\ConditionalApplianceInterface. For being able to do so, the only missing event that needs to be refactored is Drupal\eca_test_array\Event\ArrayWriteEvent.
  • We then still have some leftovers to do regards data providers, such as various event subscribers like Drupal\eca\EventSubscriber\EcaExecutionAccountSubscriber, Drupal\eca\EventSubscriber\EcaExecutionEntitySubscriber, Drupal\eca\EventSubscriber\EcaExecutionFormSubscriber and some others where it would be better to have their logic covered by the according plugins as well. If we'd face different plugin implementations doing the same thing, maybe they could at least share a trait. I'm not sure whether that's to be addressed in 📌 Make all token data providers discoverable and expose them in the UI and in the ECA Guide Active anyway, feel free to ignore this point if so.

We probably have some data providers where it's not possible to provide a 100% clear definition / declarative layer, because their contents may vary by different context situations and user-defined configurations. The two where that's the case are Drupal\eca_queue\Task and Drupal\eca\Token\ContextDataProvider. But that is fine, we don't need 100% coverage or enforce all data providers being able to predict what kind of data they will provide. It's just important that we provide the mechanic for all data providers that can predict their types of provided data.

🇩🇪Germany mxh Offenburg

As per description on the project page, this module enables you to access objects defined from parent and root processes. In other words, it is a context stack, providing context for child ECA processes. What you're looking for is a globally accessible variable throughout a PHP process (or persistent accross multiple requests?).

As far as I'm aware of, there is no project currently providing such feature. But maybe it makes sense for you creating a new project that provides it.

🇩🇪Germany mxh Offenburg

As of the currently too limiting (and thus wrong) return type of Drupal\eca\Plugin\ECA\Event\EventBase::getData I set this to NW and any adjustment that was made because of that return type limitation needs to be reverted because it doesn't make sense.

🇩🇪Germany mxh Offenburg

we do have a few implementations that are not ECA event related

We need to evaluate, whether some of them can be moved into the plugins. If not, we could tag any service being a data provider (via services.yml), so that we can collect them into another service for our required discovery.

🇩🇪Germany mxh Offenburg

since the interface forces a return type of ?DataTransferObject

The interface method Drupal\eca\Token\DataProviderInterface::getData is declared to return mixed, because it is allowed to return basically anything, not only DTOs.

🇩🇪Germany mxh Offenburg

This of course requires an update hook that properly takes care of renaming any tokens in existing ECA configurations.

🇩🇪Germany mxh Offenburg

Thanks for looking into this, glad you like that approach as well. I added a comment into the Gitlab issue fork regards your commit, which would be great if that could be resolved.

Looking good, shall I alter the other sub-modules then?

If you want to take this part, yes sure.

So, all the integrations will have to be adjusted already, so I'd prefer to remove the double support now.

That's ok for me granted we've got all our sub-modules covered. I marked those double-supporting code blocks with a @todo comment in the MR and they can be removed then.

However, that can be done later as well, as it should not break anything and foremost doesn't introduce any API change.

Agreed.

also spotted a naming convention...

Thanks for creating a separate issue for that topic.

🇩🇪Germany mxh Offenburg

Think I found a possible solution how to achieve the appliance behavior in the Processor service: The plugin manager for ECA events may provide a method for getting the according plugin ID of a system event. Then the processor can call a static method of the plugin's class, i.e. without the need to instantiate a plugin object on that early step. Created an MR390 that includes the approach.

In that MR I already refactored the eca_access submodule's events so that their logic of being a data provider and for conditional appliance got completely moved into the according plugin implementation. I didn't already refactor all other sub-modules, as this currently exceeds my time capacities.

For the intermediate phase, where it may be possible that either the system event or the plugin event implements being a data provider and/or conditional appliance, I decided to support both scenarios in this MR. It might make sense to support both scenarios even until 3.0.x, because it's a huge effort to refactor all existing implementations. If we'd want to enforce changing everything, then we'd also need to enforce other integrations and for that we'd need to remove the according interfaces entirely or move them into the plugin namespace.

When refactoring: For very different logics of data providers and conditional appliances, it might make sense for a plugin implementation to use separate child event plugin classes for it, instead of bloating up one plugin class with different cases. For such a situation, I think it's possible that you can manually define a 'class' entry within each entry of an implementation of Drupal\eca\Plugin\ECA\Event\EventInterface::definitions where you can point to a specific FQCN of a child class. I was already considering such a way in the eca_access refactoring example, but I think for that module it's still fine so I left it in one plugin class.

🇩🇪Germany mxh Offenburg

This might be a potential blocker for 📌 Make all token data providers discoverable and expose them in the UI and in the ECA Guide Active because this change will introduce a general mechanic for event plugins being automatically included for conditional appliance and for being added as data provider.

🇩🇪Germany mxh Offenburg

The simple widget plugin's implementation has an Drupal\inline_entity_form\Plugin\Field\FieldWidget\InlineEntityFormSimple::isApplicable method implemented as follows:

/**
   * {@inheritdoc}
   */
  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    $handler_settings = $field_definition->getSettings()['handler_settings'];
    $target_entity_type_id = $field_definition->getFieldStorageDefinition()->getSetting('target_type');
    $target_entity_type = \Drupal::entityTypeManager()->getDefinition($target_entity_type_id);
    // The target entity type doesn't use bundles, no need to validate them.
    if (!$target_entity_type->getKey('bundle')) {
      return TRUE;
    }

    if (empty($handler_settings['target_bundles'])) {
      return FALSE;
    }

    if (count($handler_settings['target_bundles']) != 1) {
      return FALSE;
    }

    return TRUE;
  }

So maybe you are either referencing to no Storage type at all or are allowing more than one type? Nevertheless, this is most probably a problem of Inline Entity Form module and not Storage Entities. Closing as duplicate in favor of 🐛 Storage Entities - Simple not available for storage entities referenced via storage entities Active . If for some reason this is actually a problem originated by Storage Entities, feel free to re-open.

🇩🇪Germany mxh Offenburg

That's great to hear, and thanks for looking into this. I'd also vote for merging this. Then also further refactoring can be addressed, such as moving ECA-specific logic like ConditionalApplianceInterface and others completely into the according ECA plugin implementation.

🇩🇪Germany mxh Offenburg

Great, thanks for the quick feedback.

🇩🇪Germany mxh Offenburg

Created an MR that contains the naming change. If this is fixed, I'll change the project page contents accordingly.

🇩🇪Germany mxh Offenburg

So we just change name and description of the module?

🇩🇪Germany mxh Offenburg

Thanks for this suggestion. I'm not sure whether that will help, but could be. However this would also mean an effort in renaming the modeller class (currently named as Core) plus chaning the modeller's routing URL. So this would be a major change with extra efforts required. I currently don't have the resources for such a change.

For a short term band-aid, I have added a description block on the project page to inform readers about what this module is about and what is more recommended (including a link to the bpmn_io module). Hope that helps a bit. But seriously, users should read project page descriptions and readme files. Otherwise how should they learn what it's about?

🇩🇪Germany mxh Offenburg

This should now be fixed with the last commit. If not, feel free to re-open this.

🇩🇪Germany mxh Offenburg

Created a first MR draft that unifies the three initialization layers mentioned in the IS, by using one state value. Within that unification refactor, I renamed some (newly introduced) methods and commands, as it seems to make more sense being named that way.

It also seems that we can get rid of the event's applies method entirely, and solely rely on the lazy event appliance check. Of course this is a major API change that requires any integration to adapt their event classes. However that will happen anyway when we plan to move such logic into the event plugin implementation instead. Probably makes sense to see this issue as a blocker for the event plugin refactoring when we want to make this change.

The EcaStorage class is now way simpler and does not contain any event initialization logic, this is now completely covered by the Processor, where it belongs to. Also removed the memory cache implementation for event objects from the Eca configuration entity, because it doesn't make sense to cache them when looking into the code parts that invoke /ask for ECA event objects. This reduces a bit of I/O overhead because no cache tags are involved anymore for those objects.

🇩🇪Germany mxh Offenburg

Committed, thank you for contributing.

🇩🇪Germany mxh Offenburg

Sorry, there are two numbered lists in that comment. I meant following:

The processor service that loads all event objects from an ECA config: foreach ($eca->getUsedEvents() as $ecaEvent) {

Think we can just address it separately.

🇩🇪Germany mxh Offenburg

I wonder if we should enhance \Drupal\eca\Entity\EcaStorage::resetEventSubscriber such that without providing arguments, this method would do its work even without comparing $orgEvents !== $newEvents.

Don't see any use for that, except for error situations (which should hopefully never happen). But yeah why not, like you already implemented should be fine.

Really wondering whether we could address the three points raised in #18. Especially regarding point 3 of #18, I don't like that all event objects of an ECA configuration entity need to be instantiated so that you know whether it is even relevant for the particular event. Seems like another resource overhead that may be worth to be optimized. But could also be addressed in another follow-up.

🇩🇪Germany mxh Offenburg

Could also move any ECA-related interface logic into the event plugin, such as ConditionalApplianceInterface. Could also be addressed as another follow-up then. Benefit would be that system events can then be left untouched from ECA-specific things.

🇩🇪Germany mxh Offenburg

I tried to use cache for a long time and always rand into a circular dependency injection issue.

Can confirm this, that's a bummer. Using state seems to be the only viable option for now.

Continued on the MR in one commit (see above), feel free to revert it if you don't want it to be there.

Changes I've applied there:

  1. Removed EcaBase completely to enforce existing integrations making use of the new API. Think it's better to produce a fatal to let them know that the API needs to be adapted instead of silent failures (which would mean no execution at all).
  2. Removed the lazy event subscriber, because looking at the event listener's logic in ECA for a second time - it doesn't make sense to provide two layers of event subscribers here. Instead I renamed it to be a "dynamic event subscriber" which holds a "lazy-like" invokation of the processor's execution logic when needed.
  3. Processor and EcaEvent objects are now making use of EcaEvent::execute method. With that mechanic, the system (Symfony) event object can be passed to the event plugin instance. And when that is available from within the event plugin, the event plugin is able to work with that object, for example to provide token data (may be relevant for 📌 Make all token data providers discoverable and expose them in the UI and in the ECA Guide Active )
  4. Created a new base class for ECA execution-related events. It is likely that most of them may be removed anyway in the future once token data providers are completely handled by the according event plugins. But for this change here it may be fine.
  5. Provided an upgrade path that writes the state value by implementing an update hook.
  6. Added lock mechanic to minimize a potential concurrency problem when writing into state, implemented within Drupal\eca\Entity\EcaStorage::resetEventSubscriber.
  7. Removed the facade classes of eca_misc entirely and moved their contained logic into the event plugin implementation, which is solely about providing token data. For now we have no alternative for event preparation, but as argumented in #16 this should be fine. Also now that we enable more involvement of the event plugin, we might not need such mechanic at all anymore.

I think with these changes, all points in #16 were addressed, with exception of

Clean up usage of \Drupal\Component\EventDispatcher\Event|\Symfony\Contracts\EventDispatcher\Event. In D10 this is now simply \Symfony\Contracts\EventDispatcher\Event

IMO this would add even more changes, and it's worth to be addressed as follow-up: 📌 Cleanup usage of Event classes Active

Further notes:

When introducing this change, we'll have 3 initialization layers:

  1. The event subscriber that reads from state
  2. The ECA entity storage that reads from cache (EcaStorage::$configByEvents=
  3. The processor service that loads all event objects from an ECA config: foreach ($eca->getUsedEvents() as $ecaEvent) {

Think this initialization process can be further optimized. For example, it may be considered to just put in all initialization-relevant values into one array and put it into state. That might reduce some I/O (point 1 and 2) and object instantiation overhead (point 3).

🇩🇪Germany mxh Offenburg

As we came up on this in Performance optimization: dynamic event subscription Fixed we concluded that it most probably makes sense to make the event plugins the proper place for providing token data.

🇩🇪Germany mxh Offenburg

Setting to NW due to failing tests.

Took a brief look into the MR and found a couple of problems:

  • Using Drupal's state service is not ideal for storing the list of events, because it adds a second source of truth besides the ECA configurations. Plus there is a potential concurrency problem when multiple users save ECA condigurations (one can overwrite the other's state value). The patch I linked in #7 is just using state because Rules already uses it. For performance and integrity, I highly recommend to use a regular caching approach instead, as described in #7 by making use of cache.backend.chainedfast. That caching approach doesn't require any upgrade path, while using state would require an upgrade path due to the missing state value for existing sites when switching from 1.x to 2.x.
  • The MR currently doesn't allow an integration module to provide its own event subscriber, or at least a way to add its own logic for event subscription. For example, the eca_misc module needs its own logic for facading event objects. In the MR you moved logic of that particular module up into the src/EventSubscriber/EcaBase class, where it clearly does not belong to as it breaks stable dependency principle (eca is basically using classes of eca_misc). We need a way that integration modules like eca_misc are still able to include their own logic.
  • The LazySubscriber should not instantiate a class called EcaBase, because base classes are usually abstract classes.

When this API change comes in, it needs to be at least made sure, that even when existing integration modules still do it the old way, that events are not being subscribed more than once (like the lazy subscriber is subscribed plus the integration module subscribed). This could be done for example by removing/renaming the src/EventSubscriber/EcaBase class so integration modules are forced to change their integration logic. Or we provide a way so that both the old subscriber mechanic still works and the new lazy approach, for example by introducing another option in the event plugin definition.

🇩🇪Germany mxh Offenburg

any chance for you to work on the lazy event subscriber?

Sorry, I'm not available. You can have a look at the linked patch in #7 how it could be implemented.

🇩🇪Germany mxh Offenburg

Would that also mean that all the ECA integration modules would not require any EventSubscribers any longer?

Probably yes. Could also consider making it optional, in case an integration can't get around providing its own event subscriber for whatever reason.

Sounds like we should then move the DataProviderInterface to the ECA event plugins as the only remaining commonality, right?

Probably yes, in case the plugin instance is already available at that point. Haven't checked the processor implementation in detail. At first thought the plugin sounds like a proper place. Thinking this further, if DataProviderInterface would also have a declarative method (telling the system what tokens it provides), then this approach might also be suitable for action plugins.

🇩🇪Germany mxh Offenburg

Ah, just seen that I accidentally posted #7 in the wrong issue, reverting. It was meant for 💬 How to disable single checkbox and set default values in multi-value checkboxes form elemen Fixed , where I copy-pasted the comment now.

🇩🇪Germany mxh Offenburg

Just seen that the OP also asked for how to set default values on a checkboxes field. Attached an example model for that.

Unfortunately, I encountered a bug while trying to set default values on a checkboxes fied. A fix for that is available in the linked issue. You'd need to apply the fix in that issue first, otherwise the attached example won't work.

🇩🇪Germany mxh Offenburg

Just seen that the OP also asked for how to set default values on a checkboxes field. Attached an example model for that.

Unfortunately, I encountered a bug while trying to set default values on a checkboxes fied. A fix for that is available in the linked issue. You'd need to apply the fix in that issue first, otherwise the attached example won't work.

Production build 0.69.0 2024