Sorry for the inconvenience, I'll investigate.
I won't be able to do that, but I can see that it would be valueable.
I've rebased the MR to also contain the security fixes from last night. One of the 3 test failures disappeared. The first test failure is a missing "Save" button that I can't reproduce.
The page deprecation is https://www.drupal.org/node/3458593 → , you need to update your node templates to not check for page.
For that one, we would appreciate a hint on what might be wrong in the templates, @berdir. We've checked but could figure out what exactly would have to be changed.
Thx @phenaproxima and @penyaskito, this is now fixed.
jurgenhaas → made their first commit to this issue’s fork.
Ah, thanks. I got confused because you reopened the issue.
@bigtomfelix if you still have that issue, please provide a small model with which we can reproduce.
That's not a valid option, TBH. If a user needs to manually set something so that notifications get created, it will never ever work reliably. DANSE has been built to take that responsibility away from the user. It follows business logic and makes sure that notifications get created when they need to.
If you need something where a user decides when to create a notification, then DANSE is not the right tool for that.
This is certainly not an issue for the DANSE module because DANSE does not send notifications at all. That's not its business.
The push framework is an option to send notification, including those that have been created by DANSE. And over there, an issue for digesting notifications already exists, see #3217265: Bundling / Digest of Messages → . It probably needs somebody to start working on it.
@dww fair enough. At this point, we're just cleaning up the Gin code to remove all contrib related extras, as those cannot go into core. If a contrib module wants to be rendered the same as before, it needs to take care of it.
However, there is some improvement to the admin theme in core: we're handling content entities more generically, so that they all behave the same. That should also cover Group edit forms. That said, this hook may actually be redundant then.
@coaston please provide an ECA model which is as small as possible and lets us reproduce this issue.
Great finding, this may actually be a bug in either ECA, Modeler API, or BPMN. I need to investigate.
Background: the action plugin \Drupal\easy_email\Plugin\Action\SendEasyEmail is provided by the Easy Email module. It is not configurable itself, but it declares type = "easy_email" in its annotation, which tells ECA that this plugin requires the entity field for input. It appears that ECA correctly recognizes that because we can see the field in the UI, but it isn't properly handled when the values are being returned. I suspect, this issue is in BPMN as it may ignore this one as it is not an instance of ConfigurableInterface, which is checked in \Drupal\bpmn_io\Plugin\ModelerApiModeler\BpmnIo::configForm
Welcome to ECA, @publishing future
All that ECA can do is to respond to events and then deal with context data that's been made available by that event.
So, when you start with the "Insert entity" event, that cannot distinguish what actually created that new entity. And I doubt that feeds makes any global tokens available that could be used to find out either.
That said, this either needs an event that's dispatched by the Feeds module when it created a new entity, followed by ECA integration into that event. Or, you get your feed import to set a hidden field to a specific value that your ECA model can check for when it deals with new entities.
I still can't reproduce that behaviour, not with ckeditor_media_embed, not with ui_patterns, not with syslog - all of which are mentioned in the circle in #26.
The MR!15 isn't anything to consider. It introduces massive changes, and tests are failing.
Before we should spend another minute on this one, we need clear instructions on how this can be reproduced.
There are 3 functional javascript tests failing, everything else is green. The failures are:
Inline Block (Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlock)
✔ Inline blocks
✔ No layout save with discard_changes
✘ No layout save with revert
┐
├ TypeError: str_starts_with(): Argument #1 ($haystack) must be of type string, array given
│
│ /builds/issue/drupal-3556948/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:115
┴
✔ Inline blocks revisioning
✔ Inline blocks revisioning integrity
✔ Deletion
✔ Access
✔ Add work flow
✔ Add inline blocks permission
✔ Edit inline blocks permission
✔ Inline block parent revert
Admin (Drupal\Tests\admin\Functional\Admin)
✔ Default admin settings
✔ Dark mode setting
✔ Accent color setting
✔ Focus color setting
✘ User settings
┐
├ Behat\Mink\Exception\ElementNotFoundException: Button with id|name|label|value "Save" not found.
│
│ /builds/issue/drupal-3556948/core/tests/Drupal/Tests/WebAssert.php:159
│ /builds/issue/drupal-3556948/core/tests/Drupal/Tests/UiHelperTrait.php:75
│ /builds/issue/drupal-3556948/core/themes/admin/tests/src/Functional/AdminTest.php:123
Entity Filtering Theme (Drupal\Tests\system\Functional\Theme\EntityFilteringTheme)
✘ Themed entity
┐
├ Behat\Mink\Exception\ExpectationException: Current response status code is 500, but 200 expected.
│
│ /builds/issue/drupal-3556948/vendor/behat/mink/src/WebAssert.php:888
│ /builds/issue/drupal-3556948/vendor/behat/mink/src/WebAssert.php:145
│ /builds/issue/drupal-3556948/core/modules/system/tests/src/Functional/Theme/EntityFilteringThemeTest.php:161
┴
1 test triggered 2 deprecations:
1) /builds/issue/drupal-3556948/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52
'page' is deprecated in drupal:11.3.0 and is removed in drupal:13.0.0. Use 'view_mode' instead. See https://www.drupal.org/node/3458593
Triggered by:
* Drupal\Tests\system\Functional\Theme\EntityFilteringThemeTest::testThemedEntity (2 times)
/builds/issue/drupal-3556948/core/modules/system/tests/src/Functional/Theme/EntityFilteringThemeTest.php:145
2) /builds/issue/drupal-3556948/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52
'page' is deprecated in drupal:11.3.0 and is removed in drupal:13.0.0. Use 'view_mode' instead. See https://www.drupal.org/node/3542527
Triggered by:
* Drupal\Tests\system\Functional\Theme\EntityFilteringThemeTest::testThemedEntity
/builds/issue/drupal-3556948/core/modules/system/tests/src/Functional/Theme/EntityFilteringThemeTest.php:145
I've tried everything that came to mind, I can't figure out what's wrong with them. Any help would be much appreciated.
To address the concerns about the user module and the special login handling, we've just removed that part from the MR and propose to deal with that afterwards back in the original issue at 📌 Admin Theme: support admin theme login Active .
Re-opening this one as the modern login support should be handled separate after the Admin them has been merged into 11.x
This is great. Please wait for 📌 Merging Gin as Admin theme Active before you merge this, as the hook name may change as part of Gin being renamed to Admin.
We're getting close to a green CSS linter test. The CSS files are now all properly checked, and I managed to fix around 1.200 prettier issues semi-automatically.
What's left are 38 prettier issues that are related to colour syntax. This goes beyond my pay grade, that needs to be checked and fixed by an expert.
Regarding the concerns in #26:
The MR is huge, can't argue that. What would have been a better approach to this then?
I saw that the user login theme negotiator issue was closed in favor of this, making this even bigger and also including changes to non-experimental extensions. Do we really need that feature in the initial merge of this?
The reason for handling this change here as well is that none of the two separate MRs would ever pass tests in isolation, that's why they can only come together. And the reasoning for why this is needed in the first place has been quoted from the Slack discussion with @lauriii.
I'm now working through the PCSS/CSS diffs and get them sorted.
Went through all the cspell strings and managed to either ignore them or to rename them. This test is now green.
So, we're down to CSS test being the final one that fails.
Thank you for the updated baseline @longwave, that's amazing.
As for CSS and JS, I'd leave that to Sascha and others to decide, as I'm not really the expert here.
But I'll look into the spelling details for each of the words to see how many of those we can get rid of, and then review the remaining list which should probably be much shorter.
@quietone 2 threads still open, one of which I can't find the answer to and the other which is taken over from Claro.
Other than that, the following tests are still failing:
- CSS and JS linting: I'm not sure they can be resolved short term while people will continue working on the deduplication task
- PHPStan: this is a deprecation from an Ajax trait and needs to be fixed there. Not sure how this failure got handled in other places where this isn't reported
- PHPUnit with PHP 8.5 is allowed to fail
- CSpell: there is still the list of words from #5 and I wonder how they should be approached. Changing class names or variable/setting keys should be avoided at this point, and that would even only address part of the problem. Shall we add asset directories of Gin in the
.cspell.json?
Thank you @avpaderno
As for the new project owner, I've spoken to the core maintainers who're working with me on the Gin migration, and they agreed that we want that project to be transferred to myself, and I'll then add them as co-maintainers.
Are we keeping the gin machine name in classes, config, settings keys, etc? There are a lot of references - and changing some of these once this is released will probably be more difficult.
Renaming classes and variables in CSS and JS is something that should be done after the deduplication of Claro components has been completed. That's all the files till remaining in the migration directory having to be merged into the files in the css and js directories. It was agreed that this can be done after merge and Sascha is going to break this into smaller tasks so that more people, and not just him, can work on this.
The toolbar issue should be resolved. It only occurred when toolbar was enabled AND navigation was disabled.
Next will look at create and update for user, then can move to content entities.
As users are also content entities, they could be handled just like that, so you hit two flies with a single stone.
Thanks @pameeela and @quietone for your feedback.
The changes in #14 seem to be missing, though.
I'll work though everything else, including that toolbar issue. And yes, working in follow-ups would make things much easier, but only if we feel comfortable with the initial MR, of course.
I'm wondering, should we add a component "Admin theme" to the core project?
Regarding cspell errors, here is the list of words that are available in settings and javascript, that we should ignore:
apng
backtosite
darkmode
eberhard
endapply
fullscreeneditor
ginter
grossgasteiger
gvar
highcontrastmode
imageapi
imce
lightmode
micha
navigationcreate
pamg
resizer
schwarz
scrollsync
smartdate
subtheming
syncscroll
tablesaw
tmgmt
toleft
toolsextra
totop
treetable
webform
xxxs
Where should they go?
This gets done as part of 📌 Merging Gin as Admin theme Active .
This has been done as part of
📌
Merging Gin as Admin theme
Active
. But the change in _system_is_claro_admin_and_not_active doesn't seem to be required at all.
Marking this as fixed as the code should be merged into core together with the rest of Gin, which is happening over at 📌 Merging Gin as Admin theme Active .
Moving this from child to related, as it has no relevance before merging.
Moving this from child to related, as it has no relevance before merging.
Moving this from child to related, as it has no relevance before merging.
Moving this from child to related, as it has no relevance before merging.
The feature flag for sticky action buttons is now implemented with the $settings['core_admin_theme_use_sticky_action_buttons'] flag that can be added to the settings.php. It allows for the following values:
never, this is the default and will not use sticky action buttons at allcontent_forms, this will use sticky action buttons for content entity forms, see 📌 Improve content form determination Activealways, this will use sticky action buttons for all forms
Here is the current logic in Gin on when sticky action buttons will be used:
Step 1: Form IDs that contain either of these strings (can be altered with a hook) will NOT use sticky action buttons:
'media_library_add_form_',
'views_form_media_library_widget_',
'views_exposed_form',
Step 2: All forms that have node_form as their base form ID will use sticky action buttons.
Step 3: The following routes (can be altered with hooks) will use stick action button:
'node.add',
'block_content.add_page',
'block_content.add_form',
'entity.block_content.canonical',
'entity.media.add_form',
'entity.media.canonical',
'entity.media.edit_form',
'entity.node.content_translation_add',
'entity.node.content_translation_edit',
'entity.node.edit_form',
'entity.menu.add_link_form',
'menu_ui.link_edit',
Step 3: Everything else will NOT use sticky action buttons.
Quick note on core's condition API: ECA originally used that API and it had unexpected side effects so that we later decided to build our own condition plugin manager. The core condition API is exclusively used for block visibility. And everything that adds to those conditions can very easily screw that block visibility area up badly. I would therefore recommend no using it here either.
We run into a related issue. We don't have basic auth enabled, but we have intranets without any content for anonymous users. Therefore, the 403 setting in /admin/config/system/site-information is set to /user/login. But that 403 causes the header test to fail as well, although the response contains the headers. But I assume that the 403 is enough for not testing in more detail.
This could be resolved by allowing the optional configuration of a patch that should be tested instead of the plain result of $host = $this->requestStack->getCurrentRequest()->getSchemeAndHttpHost();
@rajab natshah probably not. When Martin opened that issue, my first thought also was, oh, yet another one of those? But then I realised that a recursive router rebuild is not a circular reference in dependency injection. The recursive rebuild could be issued at any time, not only when rebuilding the container.
Please read the description below that field. It reads:
When using tokens and YAML altogether, make sure that tokens are wrapped as a string. Example: title: "[node:title]"
Updated the query expression top also allow INFO as something that shouldn't raise an alert.
Fixed the remaining test issues, backporting and merging. Thank you @rclemings for your contributions.
it would be great to do the CRUD operations for content entities in general, so we could act on terms, media, users, etc.
We've done just that in the Drupal piece in ActivePieces, maybe you can borrow the implementation from there? It has nothing hard-coded in there, it receives the list of entity types, their bundles, and their edit forms from the Drupal site with plain JSON:API, so that it's supposed to work with any Drupal site, regardless of their configuration.
I'm also interested in receiving and acting on webform submissions for one of my use cases.
The idea here is that ECA would be in the driver seat, as it is designed to respond to events that happen in Drupal (e.g. the webform submission), and then acts upon that. One action could then be to dispatch that event together with relevant data to external platforms like e.g. n8n. For AP we've done that through webhooks that AP can register on a Drupal site, and then ECA can leverage them for certain scenarios.
There may be other ways of doing this, but that's what we have so far, and it seems to be providing everything needed, and is easily customizable.
Thanks for testing this in #15 @svendecabooter, that makes sense, and I'M glad we're not in a catch-22.
I'm I wrong, or does this mean that there can't be a webform 6 release be available that works with Drupal 11.2.6 and also with earlier versions? Because there, we would most likely get the same exception.
This is looking great, thank you so much @rclemings for your contribution. I've just made a couple of minor corrections to fix code style tests. The 1 remaining test that fails is a Symfony deprecation in the Drupal 11.3.x context which we can ignore, i.e. I'll be fixing that by settings in the phpstan config.
That sounds wonderful, great progress.
Next steps will involve adding full CRUD operations for nodes (create, update, delete) and entity listing, aligning with the Drupal JSON:API approach.
Have you considered doing that part for content entities in general, not just nodes?
Once I get basic functions working, if anyone on this project would like an n8n account on my server for testing I can set them up.
I'm certainly interested to test this, let me know when it's time, but no rush.
I'm pretty sure a cache rebuild will resolve this.
@mandclu could you please give the dev release a try? I'd like to publish patch release with this fix and a few others later today, if possible.
I can't reproduce this. The user entity is just loading fine with search by name or email. It sounds like you probably run into a permission issue that the current user is not allowed to load user entities?
If that's not it, please create a simple ECA model to reproduce this and upload it here for review.
Oh, that's an interesting one. Surprising that this never happened before. Here is what's going on:
When installing a new module, the route provider from Drupal core rebuilds all routes. That triggers all sorts of hooks and events, and eventually, it calls the system_info_alter hook of the modeler api. I suspect this is because that Drupal site uses a view with a rest export, which collects all routes from views that also leverage some node access control plugins. That will call Drupal\Core\Extension\ExtensionList->getAllInstalledInfo() at some point, which then also dispatches the system info alter hook when building the module list.
The Modeler API implements that hook to alter the module info for ECA and other model owner like AI Agents so that their configuration path can be defined, as they don't own that route themselves, it's provided by the Modeler API. To alter those module infos, the access to the list of routes is necessary. And that's where the route building gets called recursively.
Now, to resolve this, we just remove that system info alter hook. Those configuration links only have one usage: they provide that link in the module list (/admin/modules). But it's not worth it to find a workaround for this issue only to have those links available.
Still, it's confusing that this hasn't shown up ever before. Anyway, on my way to fix this now.
Thanks a lot for all the testing. Glad we sorted this out together.
This is ready for being tested. Please update to ECA 3.0.7 and use the 2.0.x release of the ECA Tamper module. Then, rebuild the cache and afterwards this should be solved.
You asked the same question on Slack before opening this issue, and my response was this:
Custom events are not supposed to be dispatched by php code, they are ECA internal and should be dispatched by the action for it.
If you're writing custom code in php, then you can implement your own event.
The List: contains item condition requires a token name in the "Name of token containing the list" field, see description below the field. That token needs to be created first and it needs to contain the list with all the items you want to check against.
However, there might be a simpler way of achieving something similar. If you use the Compare two scalar values condition, your first value could be something like ,26122,26119,26567, and the second value would be ,[node:id],. The comparison operator would be contains. Note that in this example, both value to compare have a leading and trailing comma, that way you can avoid false positives, e.g. if your second value would just be 261.
You can use the Determining entity create access event and allow access to create any type of entity type. Just tried it with files successfully.
OK, we're not alone with this. Drupal Canvas had similar issues and solved it with new data types and config schema altering. We can implement something like that in ECA, and I've opened an issue at 📌 Add new data types and config schema alters to support tokens in more fields Active for working on it.
Leaving this one open until we can verify this against the solution provided over there.
Now I found the reason for this: the config schema.
The schema defines that the config field named value has the type float. Because of that, if we provide a token as the value, when Drupal core stores that config entity, it casts the value to a float. And casting e.g. [number] to a float results in 0.0.
So, the fact that ECA now supports config schema, causes that issue which is caused by a feature outside of ECA.
I wonder how to resolve this. We could either alter the config schema and turn all numeric values into string types. Or, we stop supporting tokens for numeric fields. As for the math plugin in ECA, you could actually turn things around by using the token in the "Data to be tampered" field and the numeric value in the "Value" field. This works until both values are represented by tokens.
The attachment is just the raw bpmn data, but not the eca model.
However, from the stack trace it looks like the ECA model has the issue that there is a form process event followed by a condition that wants to check if an entity field value has changed. In that context, there is no entity, so that's why that fails.
Why the drush command also executes that event is unclear to me, but without a stack trace for that one, it's hard to tell.
If your site doesn't allow you to either delete or edit and fix that ECA model due to that error, you may want to go for the kill switch that's described in the Troubleshooting section of the ECA Guide. That disables the ECA processor entirely and you can get into the UI and fix that model, or delete it.
The crowdsec module subscribes to the flood control event from Drupal core already, and if core dispatches that event, crowdsec takes that as a signal already.
If this module doesn't have additional functionality that would provide extra signals, then an integration wouldn't be helpful. From the name of the module I expected something extra, that's why I opened the issue. Please, feel free to close it if that's not the case.
Thank you @smulvih2 for investigating this. I'd like to suggest some of the implementation items you listed above, as the design of the orchestration module is to be agnostic of the remote platforms. If it requires additional features to make the n8n integration possible, then we should define and implement them in a generic way. The idea is to only have n8n specific code on the n8n side, not in Drupal.
I admit, this viewpoint got influenced by the fact that all of that was possible with the initial ActivePieces integration, and I don't know if and how something similar can be done on the n8n side, or for any of the other external platforms for that matter. But we should at least try.
Happy to get involved in that process, either by discussing it here in this issue, or in the Slack channel or even on another video call. Which ever you prefer is fine with me.
It's been a deliberate decision to only support Drupal 11.2 and later. Reason being that this is a project to showcase what can turn into something significant for Drupal and Drupal agencies in the future. And as Drupal 10 is already EOL as soon as next year, it didn't feel like supporting lower quality PHP code for something that just gets started would make a lot of sense. If we keep supporting old code for too long, it will hardly ever happen that we improve.
There has been a similar discussion on Slack in the #orchestration channel:
Therefore, I'd really prefer to keep the constraints and the code as is. But if there are compelling business reasons to reevaluate that, we'll certainly do that.
What's the message when the saving fails? I suspect this is about an invalid value in the operation field which is because the tamper plugin doesn't define a default value for it. To resolve this you need to change the operation to something else than addition and then back to addition, then the field gets a real value.
Similar comments as in the related issues apply here. I'd suggest that we finish them off in ECA 2.1.x first, and once that's being resolved, we can then replicate that same logic here.
This is looking like a good start but can be simplified. Also, I've changed the MR destination branch to 2.1.x, but still tests are failing. However, most if not all of those test failures will disappear if the comments I've left in the MR will have been incorporated. So, I'd start with them, and then see if all tests are green before the next review.
Hmm, that raises several questions:
- The
EntityFieldValueChanged->getOriginal()call is in line 43, how come that the exception is reporting line 38. Is there a chance that the code is not up to date? Maybe there is a patch somewhere that alters that code? - Any chance of getting a full stack trace when this happens?
- Is there an ECA model that takes action and fails while something gets disabled?
- The drush command
eca:disabledoesn't exist anymore in ECA 3. That disabling has moved to modeler API, so I'm wondering what's going wrong here?
This looks like what's being reported at 🐛 Can't make calculations, tokens are not properly resolved Active .
Not sure what the fix for the other issue is, but for now I needed to revert the change to that libraries get loaded again.
I ran into this as well, not related to mobile or desktop, though. I just figured that no libraries get called anymore. And when I revert that change from render to renderRoot, then the problem is fixed. This was introduced in
🐛
Fix empty render context
Needs work
but that doesn't seem to be the correct solution. I'm creating an MR to revert this.