@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_dispatcherservice - 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.
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.
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.
Hope I extracted the right code part from \Drupal\htmx\Render\HtmxBlockView::build which currently looks like this:
/**
* {@inheritdoc}
*/
public function build(HtmxBlock $block): array {
$build = [];
$currentRequest = $this->requestStack->getCurrentRequest();
// Verify that this is an HTMX request.
if (
$currentRequest instanceof Request
&& $currentRequest->headers->has('HX-Current-URL')) {
$simulatedContext = Request::create(
uri: $currentRequest->headers->get('HX-Current-URL'),
cookies: $currentRequest->cookies->all(),
server: $currentRequest->server->all(),
);
if ($currentRequest->hasSession()) {
$currentRequest->getSession()->save();
$simulatedContext->setSession($currentRequest->getSession());
}
// Add the simulated request to the top of the stack.
$parameters = $this->routeMatchingEnhancer->matchRequest($simulatedContext);
$simulatedContext->attributes->add($parameters);
unset($parameters['_route'], $parameters['_controller']);
$simulatedContext->attributes->set('_route_params', $parameters);
$this->requestStack->push($simulatedContext);
if ($block->access('view')) {
$build = $this->renderBlock($block);
}
// Remove the simulated context.
$this->requestStack->pop();
}
return $build;
}
Haven't verified, but maybe there is no cache context handling defined for the special HX-Current-URL header?
Maybe add a cache context as provided by Drupal\Core\Cache\Context\HeadersCacheContext like this:
// ...
$build['#cache']['contexts'][] = 'headers:HX-Current-URL';
return $build
Also be aware that Page Cache doesn't know of any cache contexts or headers 🐛 Page Cache must respect Vary, otherwise it might serve mismatched responses. Active .
I have checked whether VDE has something to do with this problem - it doesn't. The problem is within the bootstrap_barrio theme. Details can be found here: 🐛 Progress bar not styled correctly Active
I can see one possible problem here:
https://git.drupalcode.org/project/bootstrap_barrio/-/blob/5.x/css/compo...
This CSS file is empty, but the corresponding pcss file is not empty. So maybe the build process was wrong for compiling this file?
This problem exists for any Batch UI, it's not because of views_data_export. So wherever I start a batch, the progress bar looks unstyled.
So, I tend to lean towards option 2, combined with a warning in the release notes that this may break existing models.
Agreed
Should we probably introduce a flag in \Drupal\eca\Plugin\DataType\DataTransferObject to optionally turn off rekeying which should be called right before calling $list->set($index, $item); in the action plugin?
Either this, or initialize a DTO with a rekey option, or do both. Maybe then let the user decide whether rekey should happen, by adding a corresponding option to the "List: add item" action? There is another place where someone can add an item to a list, for example with "Token: set value" using token name [mylist:+]. But I think that one is probably never used by anyone. Still there may be other places where an item is being added to a list (other actions, such as merging).
Sorry for the noise, just want to briefly tell you this reconsideration and turnaround is highly appreciated. Thanks!
Took a look into the current MR and leaving some notes here.
#[HookDocumentation] is a confusing attribute. Its primary purpose is outside of what is relevant for the runtime of basically all running sites, yet it will demand computation resources from all of them, because attributes are part of reflection-based class discovery. Those *.api.php files were not really demanding such amount of computation resources on every site, as opposed to this concept.
Looking at the currently suggested implementation of #[HookDocumentation], the constructor argument callable $documentation is confusing. It would indicate one may pass along whatever is callable (first assumption was maybe a function returning documentation?). But what is supposed to be finally passed along, is not even callable. There is no real function like "hook_form_FORM_ID_alter".
What about #[HookInvolved]?
Regardless, AccountEvents::SET_USER seems the better approach, as it will register the session user right when the current user session is being set by core.
Whichever component in the system comes first, would "win". We have no guaranteed way that this is always the very first thing happening when setting the current user.
And that's what documentation tells, what this token is supposed to provide. Or do you disagree with that statement?
I don't necessarily disagree with it. I'd also like to have the "real session user" all the time if we could get it in a guaranteed manner. But we simply don't have it as described above.
Any other component could decide else what is supposed to be the "session user". Example: The Masquerade module
I think ECA should do what's possible within its scope, and I think the described way in #16 is still the "most" safe one I can currently think of. To recap it's the following way:
When ECA is being executed on a root-level process (= execution is not a nested subprocess), it sets the current user's entity object as session_user token. When the last ECA process on a root level finished execution, the session_user token may get removed from the token variables environment to avoid further confusion.
But I'd be happy to discover better available options.
This issue is clearly a bug, I'm wondering why it got changed to a feature request.
Because that's what the documented purpose of the session_user token is: knowing the original session user. Does Access Policy API expose that?
This may be more reliable to set the static variable \Drupal\eca\EventSubscriber\EcaExecutionSwitchAccountSubscriber::$sessionUser to the real authenticated user, which is what this variable was designed to be there for.
The only thing ECA can ever know, is the currently set user before ECA itself is executing anything. Which includes switching the user inside of ECA process execution. There is no safe way to always guarantee that we have the "real session user" at that point.
For continuation I try to summarize this issue so far:
The current behavior of ECA is:
- When ECA is being executed the very first time, it sets the current user's entity object as session_user token. On every ECA execution at the same runtime later on, the same instance of that initially determined session_user is being used.
This behavior is wrong as described in #9.
To resolve this, my previous assumption to just use the AccountProxy object was wrong. Because the AccountProxy may then of course hold the "current user" set by ECA, which is not what we want to be stored there.
I think the correct behavior would be:
- When ECA is being executed on a root-level process (= execution is not a nested subprocess), it sets the current user's entity object as session_user token. When the last ECA process on a root level finished execution, the session_user token may get removed from the token variables environment to avoid further confusion.
What "else" would switch the user account on purpose?
Access Policy API exactly does this within Drupal\Core\Session\AccessPolicyProcessor::processAccessPolicies:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
Maybe the following code part makes it more obvious: https://git.drupalcode.org/project/eca/-/blob/2.1.x/src/EventSubscriber/...
public function initializeUser(ConfigFactoryInterface $config_factory, AccountInterface $current_user): void {
if (self::$initialized) {
// Already initialized.
return;
}
self::$initialized = TRUE;
// ...
}
So this logic is applied only once for the whole runtime. Given my possible case in #9 this may even be dangerous as the current user at the very first execution may be a privileged user and this would remain the same one on all subsequent processes on the same runtime.
I wouldn't have created an issue if I wouldn't have a problem. I took the time to analyze this, give an explanation and re-reading my explanations I think they makes it obvious to make sense what the problem is about.
Also, the very first ECA execution on runtime may happen within a place where a component may have switched to a different current user on purpose. Creating an overview for access reports is a good example for that where you may check access for a privileged user and another time for a non-privileged user.
it would probably make more sense to provide the "Available and used plugins" overview by the Modeler API.
The plugins being used are defined and resided within eca.eca.* configuration entities. Modelers may not necessarily have anything to do with the plugins. Why would the modeler API to be concerned about ECA plugins? Feels more like a report sub-module to me. In the future we also might want to provide more debugging techniques that make it easier to debug running ECA processes.
My explanations are correct, especially as analyzed in #3: It's a static property, being initialized once for the whole runtime. When switching the current user with account_switcher service, an AccountProxyInterface object automatically refers to the correct current user.
The "session user" is a representation of the current state of the "current user". And the "current user" can be switched using core's account_switcher service. That may happen, otherwise that service wouldn't exist. I'm using it for a access report page, where I check access for various user accounts and show the results on one page. But that report currently does not produce correct results when ECA is involved in any way.
The code part I linked in #50 is referring to this issue. The code change was applied in the scope of this issue. Therefore, this issue is about clarifying that code part.
I now take the time to explain what the code part does that was disabled within the scope of this issue. Here is a comment that describes what it does: https://git.drupalcode.org/project/eca/-/blob/2.1.x/modules/form/src/Hoo...
Only needed when coming from the inline_entity_form module, because the embedded entity does not automatically hold most recent form input.
That means, the embedded entity is not automatically updated on its values when further Ajax is happening on the form. That code part has made sure that the entity values were updated in order to properly work with most recent submitted input used on the entity.
Here is the comment that refers to this issue: https://git.drupalcode.org/project/eca/-/blob/2.1.x/modules/form/src/Hoo...
So as long as that code part has not been clarified and properly fixed, this issue is not fixed.
This is setting a static property once, using an entity object that won't get exchanged once the current user is being switched:
self::$sessionUser = $storage->load($current_user->id());
On the other hand, the purpose of an AccountProxyInterface object would automatically use the currently logged in user.
Please see #51 🐛 ECA Form breaks complex IEF widget Active
Just create your own module that integrates the provider of your choice. Similar project: https://www.drupal.org/project/ad_entity_smart →
Also note that the maintenance status of this module is not set to be actively maintained anymore for new features.
Maybe lookout to integrate with a modern alternative instead. Example:
https://www.drupal.org/project/ad →
.
mxh → created an issue.
Maybe I got this wrong, but I understood option 1 to be an additional "Method" option in the existing "Entity: set field value" action, not about defining a new action.
There are some PHPCS complaints that should be addressed.
Thanks for addressing the feedback.
The logic is still a bit different than before when we don't use the option. For example the $path parts are being split using the explode function and if we have not specified any path position, the following is being returned now: return '/' . implode('/', $parts);
Despite the logic not being the same anymore we may set this to NR for now as the end result may still be the same (but I'm not sure).
Yes, I think we can extend eca_form_field_set_value that we change the '#value' (similar logic as the eca_form_field_default_value action) of the target $element. Meaning when $element = &$this->getTargetElement() returns a non-empty array. Maybe not even necessary to check the source event, rather just set the key on the $element if it's available might be the most flexible option.
I also tried the set submitted value action, but couldn't find the right event for it to do what I was looking for.
You're right, it doesn't seem to work for the rebuilt form to show the newly set value. So we might need to look for a way to provide a more convenient way to achieve it.
Thanks, now when using the internal path, we may want to use a proper naming of the option for that. Instead of "Skip language prefix" it may be called "Use internal path" as option, with a description something like "When using the internal path, added prefixes such as language code will be ignored. For example, both "/en/node/1" and "/node/1" would return "1" when index is set to 1.". This should make it more clear and also be understood for other cases than language code prefix.
Tests and code checks are currently all red, but that doesn't seem to come from the MR itself, rather from the current state of the 3.0.x branch. Once the section above got addressed we may set this again to Needs review.
mxh → created an issue.
Thanks. You've removed the option "skip_language_prefix" in the last commit, but this option is needed - could it be re-added?
The new logic should also only apply when this new option is in use, otherwise the previously implemented logic should just be applied as it was before, so we don't break existing configurations.
Thanks for addressing this issue.
I briefly looked into your current MR and suggest considering a different solution path. Instead of brute-force checking whether we might have a language code as prefix, may only solve this particular use case but not anything else that may add a certain prefix to a given system path. Also, there even may be routes that could actually start with a language code as prefix but were not added by the translation system...
Here is a suggested way to get the "correct" path we want to finally work with:
$current_url = Url::fromRoute('<current>');
// We want to get the path without language prefix, therefore use internal.
$path = $current_url->getInternalPath();
$path_args = explode('/', $path);
// ...
Thank you, this works perfectly for my implementation.
The action is called "Form field: set default value" and is not supposed to overwrite an already submitted value. We're using this action a lot and changing it in the suggested way will for sure break our existing configurations.
When you want to manipulate from what was submitted, then "Form field: set submitted value" is more likely the proper action you're looking for. However it might need to be executed in the right form event before values are being set in the form elements by the Form API.
Thank you for addressing this issue. I came to the same findings as you mentioned in #4 that this was changed as a part of a major refactoring without considering the consequences of this new explicit return type.
So let's change the return type here, but not the implementation.
Yes, that's totally sufficient for being able to implement a "real lazy" plugin collection.
The most important information for you on the module description page is the following:
this module is not recommended anymore
Practical usage has shown this module is causing slow PHP processes and too much memory.
#3303624: Seeing ContextStack show up in the slow php-fpm logger →
#3283297: Maximum function nesting level reached →
Maintenance status may be changed to unsupported entirely in the near future.
Until now, the starting point as mentioned in #47 is always:
Drupal with standard installation profile
Just using the current stable versions of Drupal core and every most recent version of involved contrib module at this time.
Maybe there's a difference in the combination of the theme being used, or even other contrib modules involved since Drupal CMS involves a lot more. So I recommend using Drupal core with standard installation profile as a common basis for this issue.
With my patch, you get the correct case study loaded when you edit, and if you add a new entity it gives you a fresh blank form.
Since the previous state of the MR didn't work for me as described in the previous comments, and I took the time to make it work for the standard installation profile (as mentioned), taking care that it doesn't break existing test coverage, the more interesting question to answer now is, whether the current state of the MR also fixes the problem with the steps to reproduce as you described in #55.
mxh → created an issue.
I'm wondering what scenario leads to that being correct
When either using the simple widget or possibly when using any other widget implementation for IEF.
I thought I saw some of those lazy-loaded, so I thought $context['items'] seemed more reliable... is that where you're getting null instead? I'm thinking this might be due to a difference in the field or widget config...
I got this error with the steps described in #47. Letting the user create a new node was enabled, but that should've been enabled by default anyway. So I basically changed nothing in the default widget config. Are you using different settings there?