Gottmadingen
Account created on 1 August 2007, over 16 years ago
  • Founder, Solution Architect, Senior Developer at LakeDropsΒ  …
#

Merge Requests

More

Recent comments

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This is now implemented in MR!409 for ECA 2:

  • There is an event Alter entity view mode which can be limited by entity type
  • That event automatically provides tokens, that contain the current view mode. Those are called [event:view_mode] and [entity_view_mode], both contain the same value, and they can be used e.g. in the string comparison condition to check for the current value.
  • There is also an action Entity: set view mode which allows setting another view mode. This action only works after the new event above. In another context, it would not execute.

Please give it a try and let us know if it works for you.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

What do you mean with Web ui?

When you open the MR in the browser, you get to the web UI and there you can see the comments inline in code, and in this case also the code suggestion that I made. There is a widget where you can just apply that suggestion without having to edit your code in the editor.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

A new aspect to this might be a "Feature Flag API" which is being discussed for Drupal core at ✨ Add an API for feature flags Active , which was triggered by some BC discussion in πŸ› [Views] Table sort class & indicator for active field is wrong. Needs work but could also be used for what we've discussed in here.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

There is still redundant adding and removing of slashes in the Api example. I've added the code suggestion in the MR which seems a bit simpler. It is possible to apply such suggestions in the web UI, btw.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I've had another look and realized that Drupal core removed support for textgroups at some point in time. I've had to move this into context instead, this should now be working again. Just published a bug fix release for that, thanks @hommesreponse

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This looks great with one comment where we can simplify the Api version. You're right, as it now stands, no helper function is needed.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This sounds awesome!

Is it reasonable to integrate that with PluginManagerInterface such that each plugin is treated as a feature which is on by default but could be turned off, if certain sites don't want some of them being available? Examples could be field types, widgets, actions, and all the others.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

It's the same action to set field values that you've already used for body and title. This time you have to use the name of the field that holds the reference to the paragraph and as the value you use the entity, that you hold in the token, i.e. [nuevoparagraph].

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

If anything, this would be called an allowlist. We don't want to use anything black and white.

The original post in this issue was about an issue with reverse proxy IP addresses, which must have been misconfigured; otherwise that wouldn't have happened.

The new use case brought up in #5 is about internal users who should be allowed to request invalid URLs. I can't follow that logic. If something like that happens unintentionally on a website to admins or editors, then something with that site is entirely wrong and should be fixed.

If this happens in a local or a test environment, then it's not recommended to have CrowdSec enabled there. This is a module to protect live sites. In other environments it should be disabled.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I was looking at the picture and the 3 steps look OK but incomplete. In step 2 and 3, a paragraph entity is being created, and a field value is being set to that entity. But that's it, nothing happens to that paragraph entity afterwards, so it is just gone after that.

If your intention was to add this paragraph entity to a field in your communication node, then you would have to save that paragraph entity and then add that entity to an entity reference in the node.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Nice, thank you. I'm following that other issue now, so if any questions arise, I'll be able to comment there if I can. Setting this one to "Postponed ..." and add the other issue as related.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Thanks for raising this question @AlfTheCat. First, let me not that all 3 (paragraphs, eck, ief) are actually the same: it all is IEF, so we can focus on that single component for the discussion.

Should we try and interfere with the submission process of IEF? My opinion is a strong NO. That whole process is already as complex as it can get. Hooking into that can only cause trouble.

Of course, you have a valid use case here. I'm sure you're not alone with that, there are certainly many others having similar requirements. Therefore, I'd propose that as a feature request with the IEF module in a way that in the complex IEF mode, there should be a config option to immediately save that entity. This would allow for subsequent components in the parent form to choose the "Select entity" instead of creating again a new one.

Consequently, such referenced entities, if they get stored immediately, they will remain in your system even if the parent form will never be submitted. But that's most likely what you want, at least in your use case above.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Thanks @rex.barkdoll for the great issue description, this makes the problem straightforward to understand. And your observations are perfect, that's precisely what's happening.

To resolve that, I see 2 approaches:

  • SimpleSAML module implements a new event that they dispatch after they've finished the role sync. Then an ECA integration could be implemented that gets to know about that event and you could use that as your starting event.
  • SimpleSAML could raise their priority, so that their hook for the user login gets dispatched first.

Both are on the SimpleSAML module and I could imagine that both are valid and would not only benefit ECA but also other use cases. From what you described, it feels as if the role sync needs to be the first thing to happen in any event. I would have thought it should even be part of the session initialization and not an after login hook. Would you mind reporting that with the SimpleSAML maintainers? We could maybe even move this issue over there, then I would stay in the loop and chime in if that was helpful in some way.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

@bradjones1 for those calling the method, that's OK. But there is e.g. the sqlsrv module β†’ which extends that method, see \Drupal\sqlsrv\Driver\Database\sqlsrv\Schema::tableExists and suddenly becomes incompatible.

As the abstract class \Drupal\Core\Database\Schema isn't marked internal, it feels as if extending it is what's supposed to be done.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Thanks, I'll give that a try when we get around to that project again. If it works, it's probably worth a mention on the project page and/or readme of the entity share module.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This is working very nicely already. Only a couple of related issues are left open, but the basic functionality is in place and already published on the ECA Guide.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

jurgenhaas β†’ created an issue.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

MR 408 now contains a WIP for tokens discovered from PHP attributes on a couple of methods in the ECA event plugin.

What's missing and needs some more thoughts:

  • Potential tokens provided by parent classes
  • Tokens provided by event subscribers
πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

But for test case 5 you reported "token is empty". In any event, you have a working model, and the other one contains a view which doesn't return any results. I think, that's as much as I can help with.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

No, that's not correct. Forget about ECA, you can disable ECA if you want to reproduce this. Just open the view in the views UI and run "Preview" on the view there. You can see that the one without the IDs is not returning any result. So, what broken is the view's configuration, nothing to do with ECA whatsoever.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

The locale module is part of Drupal core and it's printable name is "Interface Translation".

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This is unfortunately a breaking change for Drupal 10.3, isn't it?

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Unfortunately, this is a breaking change in 10.3.x, isn't it?

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

The commit from #33 expects a return value int|float for the getWeight method in DraggableListBuilder. I just came across a config entity, that has a weight property, but that may be NULL. In that case, this throws an exception.

Should the getWeight method maybe fall back to 0, if the weight property has no value?

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Ah, this is a field value validation on number fields. Here is what the tamper module uses for their math plugin:

    $form[self::SETTING_VALUE] = [
      '#type' => 'number',
      '#title' => $this->t('Value'),
      '#required' => TRUE,
      '#description' => $this->t('A numerical value.'),
      '#default_value' => $this->getSetting(self::SETTING_VALUE),
    ];

This generates a HTML number field, and they define number as integer. On submission, Drupal core is validating number field such that they have to be integers.

Not sure if the tamper module wants to change that field type, moving it over there for review.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I'm not a maintainer of the classic modeller, but I know that it doesn't implement the ECA API for modellers. It does state on its project page that it's really only a low-level tool. My personal recommendation: please use the bpmn_io modeller.

At least, you could try if the problem above can be reproduced there as well.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

The views in the model process_lgag7wm and bpmn_io-process_lgag7wm2 are different.

If I create a number of test or 1_test nodes, then the view from the latest model shows the correct nodes in the preview where the other one doesn't.

The 2 differences I found:

  • The broken view has only the test field in the field list, but not the ID
  • The broken view has an extra filter compared to the other one

I haven't tested further, but from that it's clear that this has nothing to do with ECA.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

@PCate yes, that looks like what's needed. I guess, most of the "tokens" are read-only and a few (e.g. "row") might be modifiable, right?

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I've checked the method in FieldUpdateActionBase and realized that the Create a map of indices that refer to the already existing counterpart. part should only be executed if the method is NOT force_clear. That makes it work correctly and also fixed the test written by @danielspeicher in #11

Then, I've also added another test in the same method, that ensures that the method clear does NOT change the field value.

While being in that part of the code, I remove the nested ternary structures as they are difficult to read and should ever be used in PHP.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Do you have the locale module enabled? This is needed since the purpose of svg embed is to translate contained text strings in the file on the fly.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

@PCate I'd be happy to address this for the forthcoming ECA 2 release. However, I'd need your help as I don't have an overview of which data would be required for which of the migration events. Could you please provide me with some input on this?

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Thanks for reviewing all this. Yes, it's a lot of extra code that we could now get rid of, fortunately ;-)

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Looked at the test and I think it doesn't actually test the problem why this fix got introduced: it's about trimming a field value.

Example: if the node title is my title with one or many spaces at the beginning and/or end of the string, and the use wants to replace that title with the trimmed version my title, then the set:clear function does NOT work, but the set:force_clear does.

So, we should have tests that confirm, for that case, that one mode works and the other doesn't.

Apart from that, there is a code style issue and the change of return type in modules/content/src/Plugin/Action/SetFieldValue.php is probably off-topic, and we may want to go through such smells (that are not reported by PHPCS or PHPStan) in a separate issue, if at all. I'm not sure how certain PHP versions behave if interfaces and subclasses have different notations of the same method.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This is now also back ported to 1.1.x

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Shall we open another ticket with that alternative approach

Don't think so, it was mentioned as an alternative approach, not additional. But I've now merged !391, so that does the job, nothing else required.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

All configuration forms are now properly tagged so that we can identify all fields that either reference a token or those that support token replacements.

All those fields get identical description texts, which is done in \Drupal\eca\Plugin\ECA\PluginFormTrait::updateConfigurationForm. There we can later add some extras, that have been suggested above.

Note, this only works for plugins where the maintainer provides the tags. We should encourage them to do that, at least there is a change record where they get informed about this: [#3381190]

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

All plugins can now have a version annotation, which will be published in the ECA Guide. That version should tell, when that plugin got introduced.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Templates have been updated.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This is now all cleaned up.

Not sure about this one:

Careful on early invokations (such as listener methods and processor's execution method), where only object arguments are being used.

Tests seem to be OK.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Moving to the separate module.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Moving to the separate module.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Moving to the separate module.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Yes, should be fixed by the other one. Closing it for that reason. @freelock please re-open if that wasn't correct.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

As the tests in πŸ“Œ Restructure tests in render module Active already contain the merge tests, it looks to me as if this is already covered, right?

But we should back port this to 1.1.x as well.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This makes perfect sense, thanks so much for restructuring the tests. This is now so much more useful.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Added note about MR pipelines.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This got fixed as part of πŸ› ECA Content knocks out "Promote content to front page" and "Make content sticky" Admin actions Needs review today. That dynamic property does not exist any longer.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This is now implemented with an action that allows to change the ECA log level for the current event context. Once ECA leaves that context, the previous log level will be restored.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This has been fixed in 2.0.x and 1.1.x

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

Fixed a code style issue with unused use statements.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I've taken a completely different approach, the entity stack from the first approach didn't work out. The reason is that the action plugins, that set field values on entities behave differently in ECA context than outside: inside ECA, the action plugin can be configured such that the entity should be saved or not when the field value has been set. For action plugins that don't have that configuration option (like core's promote and sticky actions), ECA assumes the default value, i.e. not to save the entity after changing the field value.

However, if those plugins run outside ECA, e.g. as a bulk action, then the entity needs to be saved, otherwise the changes would be saved.

Therefore, I've now added a method isEcaContext() in the ECA processor, which can tell if the current stack trace is within an ECA processing event or not. That works 100% reliable in real life.

However, this failed for some tests, since they call the set field value action plugin without going through the processor. For that special case I've now also added a state value, which simulates that context for those tests.

@danielspeicher, is that acceptable from your point of view?

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

The flag, that an entity was related to an entity event, was set too broadly. Also, the method that was used is deprecated in PHP 8.2 since we used a dynamic property.

This is now replaced by an entity stack that only gets filled, if the entity event really got dispatched. If not, our set field value class can now properly determine, that NO eca model was involved and will correctly save the entity.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I've now fixed that and also back ported to 1.1.x

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I've rebased the MR so that tests are passing again.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I wanted to address the latest bits and realized that the solution was already in place: we can already use order_item:purchased_entity:entity:weight:number as the token name, and both form validation and later the condition evaluation do work as expected. So, there is nothing to do in the code.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I've now implemented the suggested solution from @mxh which needs a review. But we also need to write a test for this.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

This is now backported to 1.1.x

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

I've now merged this into 2.0.x and will back port to 1.1.x tomorrow. If anybody needs more features later, we can then address them in separate issues. Thank you again @ergonlogic for this amazing contribution.

πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

We need more information before we can come up with a plan. Changing issue properties in the meantime.

Production build https://api.contrib.social 0.61.6-2-g546bc20