🇩🇪Germany @mxh

In #8 your first sentence was about definition, directly followed by "It would be better to target PHP free theming instead." with the followed sentence "Avoiding PHP is also the guarantee than theming will not introduce performance or security issues."

... yeah I might've got your argument wrong. Also for the definition format "YAML vs PHP" I disagree with the security argument.

mxh → created an issue.

Avoiding PHP is also the guarantee than theming will not introduce performance or security issues.

{{ user.field_name.value|raw }}

Now there is a new way to assemble a custom configuration form that contains selected parameters. To assemble the form you may access "/admin/config/parameters" and click on "Assemble config form".

Side note, the instance itself is not "autowired", but that's what "createAutowiredInstance" might wrongly indicate. Therefore I'd favor "createInstanceAutowired" but that's also really just a micro-nit-pick now 😃

Thanks for creating this issue. Although this feature request makes sense, I estimate this to be an edge-case rather than a commonly used requirement. I certainly don't have time in the near future to implement this for you. However providing a suggested implementation I'd be happy to review and commit on approval.

:(

Thank you for creating this issue and submitting a patch (merge requests are preferred).

This module is not yet compatible with D11 at all, see version constraints. Linking related issue.

Also please note the following description from the project description page:

Important: Due to resource problems, this module is not recommended anymore. Only security and maintenance fixes will be applied.

Maybe either one of

• AutowiredConstructorTrait
• AutowiredInstanceTrait

and the static method to be named as
public static function autowiredInstance()

?

I have added a convenience shortcut into the Parameter class. It should now be possible to do the following:

$value = Parameter::value('myparam', 'strict');

Also this should work now as originally intended:

$parameter = Parameter::get('myparam', 'strict');

PHPCS complaints are indeed fixed now, thanks.

Still not using correct Drupal coding standards. Example:

+  /**
+   * The decodeValues function.
+   */
   protected function decodeValues(string $values_string): array {

Committed, thanks.

Crediting.

I don't know why I keep getting in the mess where I have to maintain modules that I haven't created.

Expressing this frustration at a project that I maintain mostly in my free time. I once put the efforts creating this project. And instead of keeping it in a private repository, I did put more time into it to achieve common usability and made it deliberately available for everyone. Facing such comments is not nice. And yes, I read your original inappropriate comment.

Looking at the changes required, mostly come from breaking BC from ECA v2 to v3. Complaining to maintainers not keeping up with the ongoing development is like complaining the free beer you got isn't cold enough. If you don't like this project or its maintenance state, go ahead and fork it. Or build your own solution. FOSS allows you to do so.

Attached sample model that proves #20. Used on fresh Drupal installation with standard profile using eca_access eca_content bpmn_io eca_ui eca_base modules enabled.

How come this MR is being set to review where it's obvious that the following requirement of the IS is not covered at all:

Earlier we could set e.g. "forbidden" at the beginning of the chain and then change it for "allowed" for some conditions.

Then @a.sinista directly sets it to RTBC stating "Works as expected". What exactly is working as expected?

Since this MR does not include an update hook to update existing configs, following warning is being triggered wherever an ECA config with determining entity access is involved:
Undefined array key 3 in Drupal\eca_access\Plugin\ECA\Event\AccessEvent::appliesForWildcard() (line 293 of modules/contrib/eca/modules/access/src/Plugin/ECA/Event/AccessEvent.php).
So an update hook is needed as well if you'd want to actually use this concept - which won't suffice as already stated in #14. It seems my feedback is being ignored repeatedly so from now on I reduce spending my time on such efforts.

Also tests are missing that cover this automatically for the long run.

BTW there are inconsistencies of the behavior between existing access events.

https://git.drupalcode.org/project/eca/-/blob/3.0.x/modules/endpoint/src...
is behaving way different than
https://git.drupalcode.org/project/eca/-/blob/3.0.x/modules/access/src/E...

When you set default access as "forbidden", then it is always forbidden, no matter what you do in the successors. It doesn't make any sense. See comment #14 for details.

Answering #54:

Yes of course, the attribute constructor arguments may be defined in the ways we see to make most sense, including support of multiple entity types, default visibility settings etc.

Another option would be to annotate functions, not classes, like OOP hooks do...?! Or maybe better not...

OOP hooks make sense to be defined on methods since they address single invokable functions. Extra fields on the other hand may involve more things besides the main rendering / widget logic. One thing that may be often used is building the configuration form. Having all of that resided in one class that belongs to the extra field's functionality would be great. So using a class-level attribute seems suitable.

When using a class, the benefit might be to combine the form display, view display and eventually some kind of data handling in one class?!

This might still be possible if an extra field plugin could live or be discoverable within the same (parent?) directory. Then something like this may be possible:

#[ExtraViewField(id: 'my_extra_field', entity_type: 'node')]
#[ExtraFormField(id: 'my_extra_field', entity_type: 'node')]
class MyExtraField implements PluginFormInterface, ExtraViewFieldInterface, ExtraFormFieldInterface {

  public function buildConfigurationForm(...);

  public function formElement(array &$element, array &$form, FormStateInterface $form_state): void {
    // $element has #weight already set and is already attached to $build['my_extra_field']
   $element += ['#type' => 'details' ...];
  }

  public function viewElement(array &$element, array &$build): void {
    // $element has #weight already set and is already attached to $build['my_extra_field']
    $element += ['#type' => 'details' ...];
  }

}

However such implementation would be using the same buildConfigurationForm(...) for both view and form, not sure whether that might be a bad thing.

Anyway we may continue elaborating all this maybe in a separate issue as suggested by catch.

After some more thoughts I guess the concept of an extra field itself is still valid, even when looking at all other concepts we have now. A plugin-based approach as suggested by @anybody, maybe covering methods for optional configuration and rendering could be straightforward.

Splitting the concept up into "view" and "form" plugins might be worth it, this could make implementations get aligned to single responsibility and core may already prepare as much as it can for rendering at the place where it got placed into.

Maybe something like this:

#[ExtraViewField(id: 'my_extra_field', entity_type: 'node')]
class MyExtraViewField implements ExtraViewFieldInterface {

  public function buildConfigurationForm(...);

  public function viewElement(array &$element, array &$build): void {
    // $element has #weight already set and is already attached to $build['my_extra_field']
   $element += ['#type' => 'details' ...];
  }

}

#[ExtraFormField(id: 'my_extra_field', entity_type: 'node')]
class MyExtraFormField implements ExtraFormFieldInterface {

  public function buildConfigurationForm(...);

  public function formElement(array &$element, array &$form, FormStateInterface $form_state): void {
    // $element has #weight already set and is already attached to $build['my_extra_field']
   $element += ['#type' => 'details' ...];
  }

}

Looking at some practical use cases, I don't think computed fields would be an equivalent replacement for extra fields.

Extra fields, as elaborated already in previous comments long time ago, mostly serve UI-related things. That means in many cases you'd want to have something like a renderable array and/or Twig template involved when displaying the extra field's "value".

On the other hand, computed fields are at the level of field definitions, which could be seen as the "data layer" maybe (it's one layer between database storage and frontend layer). I also think computed fields may be exposed automatically such as JSON API since they are "real" fields. So it may come with some surprising or even unwanted side effects when trying to use computed field instead of extra field.

Looking at proper replacements, I currently think extra fields are some sort of relict from the past but at times still needed. They're almost impossible to be reused. But they may be still relevant if you still use classic "Manage display" without Layout Builder (or maybe in future Experience Builder) and "Manage form display". For example, I often have to define them when rendering further markup sections within an content entity form in-between other fields.

When using Layout Builder, I'd prefer creating blocks instead of extra fields. They can already be implemented as full plugins and reused at other places if needed. Blocks are the most close equivalent that come to my mind right now. I had SDC in my mind as well but these are a bit lower level than block plugins (you could still write a block plugin on top of a SDC if you want though). Once Experience Builder is available, it might also come with yet another concept.

@anybody not sure whether you already know, but computed fields exist in core. Here is an excerpt from entity.api.php:

function hook_entity_base_field_info(\Drupal\Core\Entity\EntityTypeInterface $entity_type) {
  if ($entity_type->id() == 'node') {
    $fields = [];
    $fields['my_module_text'] = BaseFieldDefinition::create('string')
      ->setLabel(t('The text'))
      ->setDescription(t('A text property added by my_module.'))
      ->setComputed(TRUE)
      ->setClass('\Drupal\my_module\EntityComputedText');

    return $fields;
  }
}

mxh → created an issue.

Well, then we're blocked by those problems. So, we need a separate issue for fixing those problems first, flagging this issue to be blocked by that issue. Once that issue got fixed, then it unblocks this issue and we can continue to fix this issue.

The last commit in 2.1.x branch has green sign, this MR has a red sign. Maybe those checks don't even really make sense, but anyway, they were added once before by you: https://git.drupalcode.org/project/eca_vbo/-/blob/2.1.x/.gitlab-ci.yml?r...

An MR is ready for a review once it has a green sign.

The MR has a red cross sign, something is wrong there.

I don't know what's going on with 10.5 and the missing LegacyHook attribute there. I thought the whole point of that one is to declare an existing function to be ignored once a core version is in use that is capable of real OOP hooks. I was able to implement hooks using this attribute in 10.3 / 10.4 but suddenly it's gone and cannot simply add it that way.

mxh → created an issue.

I have a question, feel free to create another issue around this if it cannot get quickly answered:

Since Experience Builder seems to be based on building frontend components using React, will it mean that once a form using Ajax is shown on a page, the it will load both HTMX (form now jQuery) plus React? Seems Drupal core currently evolves on top of two different frontend strategies.

mxh → created an issue.

mxh → created an issue.

mxh → created an issue.

@alezu ok, once you've finished the merge request and is supposed to be reviewed, please feel free to set the status of this issue to Needs review. Thanks!

Besides I doubt this approach actually works out to restore what was possible before: I don't think it's a good idea that an event is not only an event (an entrypoint) but suddenly influences the outcome / result that is usually the purpose of an action. I think this would then be the first time, that an event takes over the role of a component that actually changes something. So this will add even more complexity for configurators. It was kind of straight forward that you just had to check for the "Set access result" actions in the process configurations for what is the supposed outcome in the end. Now you'd need to calculate the actual outcome in a weird way that is hard to explain, using these truth-tables.

The truth-tables of Drupal's access systems do makes sense - on a level of sub-systems. For example, the Group module will return an access result, and another access module (for example ECA) will return another access result. Combining the received results using the truth-table makes sense. But it doesn't really make sense on the level of ECA process configurations.

If you'd want to make actual use of such a truth-table on the level of ECA process configurations, you could allow it, but it should be explicit. Example: The "Set access result" would provide an option how you want to set the result - either using a truth-table for OR concatenation, for AND concatenation, or overwrite (= the way it was before). But this approach is now nearly impossible to achieve since that change in the linked issue.

For clarification, my example of #8 is a representation to achieve the following use case: "Let's deny access by default, also for the case anything goes wrong. And then optionally open up the gates based on conditions" As mentioned, this is currently not possible anymore, also not with the idea described in #12.

@alezu thanks for creating a merge request. Is it ready for being reviewed?

For ECA, one event is being instantiated per access call. That means, when having multiple ECA configurations reacting upon the same access event, they set the access result on the same object.

https://git.drupalcode.org/project/eca/-/blob/3.0.x/modules/access/src/H...

Before 📌 ECA Access: Do not set AccessResult in applies() Postponed it was possible as was described in #8 and you could also just set a default access on certain conditions. It was a mechanism we have used intentionally in hundreds of configurations and we heavily rely on it. Now it's not possible anymore.

What if you have one ECA config (weight 0) that sets access forbidden for anything by default, and another ECA config (weight 1) that sets access to allowed based on some conditions - will it work?

This won't really help. Once forbidden, always forbidden thanks to 📌 ECA Access: Do not set AccessResult in applies() Postponed .

"Seeking new maintainer" is the current maintenance status of this project. Shall it be changed now?

It will likely cause breakage of existing processes, and to the worse it will break without error, but logically since the inline entities won't be provided with most recent form input anymore. But since this mechanism is so unreliable, it may be worth getting rid of it.

This is not working at all in D11.2.

Maybe remove the whole implementation of Drupal\eca_form\HookHandler::inlineEntityFormAfterBuild ? Seems it wasn't working well anyways. despite it may be nice to work with the most recent form input on the entity directly, which is way more convenient - it wasn't explicitly requested anytime either.

mxh → created an issue.

This documentation states that any PHP class placed inside src/Hook will be automatically registered as an autowired service. But above is an example that manually registers a service in node.services.yml. What's the correct way now?

Stumbled into this, experiencing the exact problem as described in #2.

I'm wondering why all that stuff is being executed in an isolated way. This doesn't allow to add relevant cacheability metadata, and attaching more assets when needed is also not possible. Yes it's kind of an edge case, but it's possible to call attach_library without any error so either it should be supported in there or an error should be thrown when attempting to do such thing.

We've created a discussion history here that isn't collaborative. The original scope of this issue was thrown away and has gotten into a mess of subjective arguments and, even worse, includes more negative personal tone than objectiveness. It's gone into a debate now rather than a discussion.

This is a public issue and not a private thread. We should try our best to communicate inclusiveness, inviting others sharing their thoughts and experience. Looking at the comment history here I can imagine some reading this will be intimidated and stay silent - not good.

As a starting point, the proper way would be to

• Restore the original scope of this issue (or even better, close this one and start a new one with the original scope). Keep the room to find possible ways for this particular scope, fix it or close it.
• Creating follow-ups when needed as already described in #44.

No offense, but I feel this issue involves too much subjectiveness which makes it too hard to ever come to achieve any objective agreement. +1 for #44.

One could argue that "breaking glass" would be writing a small patch that removes the final keyword from whatever core or contrib class you wish to extend. At that point it's both a deliberate action and plainly obvious from the patches folder that someone did, in fact, break glass.

This is my preferred way, as I definitely know then that "from here on, I'm on my own" and this way is my "spirit of open source", because
a) I can read the source code and
b) I am legally allowed to change the code

I've caught myself a couple of times, "accidentally" using services and classes that are marked as "@internal" (Drupal\Core\Render\Markup, anyone?) so I can already see myself "accidentally" extending "@final" classes.

When overseeing this sort of declaration, getting blocked of a problem in there, looking for a solution, proposing the solution by creating an d.o. issue and then it being immediately closed by a maintainer as "won't fix", because "well you should've taken care of looking for the @final annotation!" I'd not be angry but certainly will think twice before my next attempts of contribution.

Sorry, closed this as I've misunderstood the intention of this issue. Since this is a question on "how to test this", I leave this visible for others willing to help out.

Thanks for creating this issue.

The maintenance of this module has been set to unsupported.

See status on project page: https://www.drupal.org/project/eca_context →
See corresponding issue: https://www.drupal.org/project/eca_context/issues/3426317 📌 Consider dropping maintenance support for this module Active

Therefore, setting to won't fix because there will be no ongoing development.

Thanks for linking to the issue about supplemental attributes.

What methods are out there in the wild?

Only custom ways I guess. For example:

• Implement the alter hook for the plugin definition. There, swap out the "class" definition by your decorator class, put the original class being decorated somewhere else - for example into a "myplugin_inner": $original_class (as mentioned this is possible with annotations but not yet with PHP Attributes). When being instantiated, the decorator then manually reads out the original class name from the plugin definition, instantiates an object of this original class definition and sets it as "inner" property.
• Alternatively, decorate the plugin manager if needed, introduce an interface such as "DecoratorPluginInterface" for the plugin implementation. When instantiating, the plugin manager checks whether current class is implementing this interface and if so, looksup into the plugin definition for "myplugin_inner".
• ... Or in the way the contrib plugindecorator module does it.

Another factor of hidden cost: Majority of plugins can be instantiated multiple times per request, whereas services on the other hand often are "shared" which means they are being instantiated once per request. So for "singleton" services this pattern may not have much of an impact, but for instantiating plugins this might be noticeable.

Many oversee this hidden cost of the decoration pattern: once the parent class implements another method or logic, the decorating class must be at least checked and refactored when needed. Many times this is not really a justified tradeoff, especially when a class provides many methods.

Even in the service workspace many modules don't follow this pattern. Instead, the class definition is being swapped out. One example is the contrib Token module: it replaces the token service with its own implementation by simply exchanging the class. So even if we have a general API provided to decorate plugins, still most modules probably won't follow up on it.

Based on these findings I don't think it's worth to introduce another API layer for decorating. Also it seems the usage of the linked contrib module wasn't that high, so this may be of interest only for very few developers.

Sometimes I wonder why Plugin API is not leveraging Symfony's dependency injection component by using service collectors for plugins and reinvents the wheel at some point.

There are already ways of decorating a plugin if you really want to. But since there are multiple ways, everyone is doing it different. Maybe we can agree on a possible "non-intrusive" general way and just document it how to decorate a plugin?

An important thing I miss since we replace Doctrine Annotations by PHP attributes is, that one is not able to add an arbitrary definition key anymore. One example are action plugins, where the contrib ECA module is adding its own meta information needed for properly grouping and documenting plugin definitions. We could think of introducing an "extra" key into PHP attributes for plugins, using a similar concept such as third_party_settings. Then we'd be able to put in class definitions that are being decorated by the current class definition of a plugin.

mxh → created an issue.

Added another small note regards the array lookup. Other than that the code changes look good. I have no time to actually test it though.

Some thoughts:

• When implementing this on one of the next major versions that require at least Drupal 11 then I guess we don't need to consider LegacyHook or any other sort of BC layer. Just going all-in for OOP hooks.
• I didn't look any more in detail, but one thing I noted was that hooks are being registered as listeners into the event_dispatcher service - something ECA does as well. Is it maybe possible that we merge some of our hook-related events entirely to OOP hooks, so we could reduce some class definition overhead?

Thank you for your kind words @mandclu, I really appreciate it 🙂

If, however, you would prefer to not even see the notifications for this module, then I will remove you a maintainer if that is your preference.

All good, we can leave it as is. Just wanted to let you know about my current availability for maintenance.

Likely this is yet another issue around ✨ [META] Expose Title and other base fields in Manage Display Active

If so, please consider closing this one as duplicate.

MR237 contains test and fix, whereas MR238 only contains the test. Running the test alone should give some fatal errors or exceptions, whereas the fix should pass the test.

I had to add another fix part into group_user_delete hook implementation, as the problem is also happening when deleting a user that is the owner of some groups.

Sorry I only had the time to work on top of 3.2.x branch, needs probably a reroll for 3.3.x (and other more recent versions).

About to write a test.

Is this automatically solved when 🐛 Node update hook is triggered upon group content create RTBC is fixed?

Thanks for the link to the other issue. Using A token for the opening bracket and another one for the closing one may indeed a valid trick to work with.

However, the "#parents" key is a common Form API concept. It's not something special. An implementation based on a certain API is supposed to incorporate its common concepts.

Yeah, this might work too. Or move the logic from the event into the Processor. DynamicSubscriber should be as "dump" as possible and only serve one purpose. Feels a bit odd to disable the processor there.

Is #21 a reply to #20 or a general information about modeler API? Maybe posted into the wrong issue (looks like an answer regards modeler API)?

mxh → created an issue.

As long as the #parents key isn't incorporated into the states definitions, this may only work for now with the bpmn_io modeler. But it wouldn't work for any other tool that embeds plugin forms "inline" such as eca_cm does. The #parents key is a common form API concept and thus may be worth to be incorporated. For details, see #12.

mxh → created an issue.

mxh → created an issue.

Ok. We may be more flexible when not supporting "BC" to the BPMN's property panel. Currently I see no reason why I'd use BPMN's property panel if I could have all the good stuff of Drupal's form API. Maybe not worth trying to cover both. But that's just an assumption around this, maybe there are good reasons to keep that compatibility I just have missed.

Do we get full form API support from this mechanism, including native form validation, submission and Ajax such as autocomplete?

Took a brief look into this and left two minor side notes.

That's great news and exciting!

Do we need to take care about CSRF token protection when opening up for a real form coming from the backend? So far my understanding is that it's needed when the request might involve some write operations. Maybe the CSRF token is already included in the rendered form, but might be worth a double-check to look into.

mxh → created an issue.

mxh → created an issue.

Setting to RTBC since tests are green and code looks like a working solution.

Looks great, thanks!

Not sure what you mean in #9 - I think calling a helper method once would be a bit more readable code rather than back-and-forth flipping of the boolean variable from the outside. Nevermind, the current state of the MR now looks also to be a working solution.

Another suggestion, since we already check for instanceof DTO: We could add a helper method to the DTO "setWithoutRekey" or "setWithRekeyOption" which takes care of properly setting the rekey state back-and-forth.

public function setWithRekeyOption($property_name, $value, $notify = TRUE, $disableRekey = FALSE) {
  $initial = $this->disableRekey;
  $this->disableRekey = $disableRekey;
  $this->set(...);
  $this->disableRekey = $initial;
}

Thanks. While the code change looks exactly as described, there is a failing test which may require some attention: https://git.drupalcode.org/project/eca/-/jobs/5327402

Setting to NW due to the failed test.

Thanks for addressing this. Took a brief look and left one note.