When you define a configurable action plugin that uses #ajax
in its buildConfigurationForm()
, the submitted changes are not saved in Action entity.
Steps to reproduce:
- Get a configurable action with
#ajax
in its configuration form. As far as I know core doesn't have such actions. I've discovered this bug while working on a contrib module. What you could do is to apply the patch "configurable-action-ajax-bug.patch" to core in order to make EmailAction use#ajax
in itsbuildConfigurationForm()
so we have a case to work on. - Go to
/admin/config/system/actions
and add "Send email" action. - Fill in the form fields (the "configurable-action-ajax-bug.patch" triggers ajax on "recipient" field upon "change" event, make sure it's triggered at least once)
- Submit the form
- Now go to edit again this freshly created configurable action.
- Expected: to see all the values you've submitted
- Actual result: the form is empty, i.e. config of your action is not saved.
Why does it happen? As far as I could debug the core, the following sequence of events happens that eventually produces the bug:
After ajax ran, the $form
is cached
When you submit the form, the form build process does not happen, since the form is pulled in from cache.
Since there was no form build, the Action entity does not initialize its plugins (something around getPluginCollection()
method)
When your configurable action plugin (such as EmailAction in this particular case) updates its $this->configuration
the changes are then not picked up by Action::save()
method. Namely, in ActionFormBase::save()
the following expression yields FALSE $this->plugin == $this->getEntity()->getPluginCollections()['configuration']->get('action_send_email_action')
when the config form ran ajax and yields TRUE when the config form did not run ajax (that is when the form was built instead of retrieved from cache).
Comment | File | Size | Author |
---|---|---|---|
#24 | 2816861-action-plugin-ajax-18.patch | 9.34 KB | jibran |
Comments
Comment #2
bucefal91 CreditAttribution: bucefal91 commentedAs far as I could identify the root cause, the bug occurs because
Drupal\action\ActionFormBase
keeps a "shortcut" link to action plugin in$this->plugin
.That property is initialized in
buildForm()
during normal (no-ajax) form process.However, that code does not run when form is cached (when AJAX is involved). The solution I propose is to simply ditch that "shortcut" property and always use
$this->entity->getPlugin()
.The patch that follows this approach is enclosed.
Comment #5
dpiLooks like this is still an issue.
Comment #6
dpiOriginal patch intact, added tests.
Comment #7
dpiThis issue affects the patch in #2797583: Dynamically provide action plugins for every moderation state change
Comment #9
dpiTest only patch failed successfully.
Updating patch with coding standards.
Comment #11
dpiThis should work
Comment #12
dpiComment #13
jibranThis is blocking #2797583: Dynamically provide action plugins for every moderation state change now.
Let's create a helper method for this.
Comment #14
dpi@ #13
Honestly seems a bit much. Taking cues from block, it goes the full route to building a form.
Attached patch with feedback from #13, though happy to go with the patch in #11 depending on third opinion.
Comment #15
jibranThanks for addressing the feedback. This is ready.
It can be fixed on commit.
Comment #16
jibranComment #17
tim.plunkettThis seems like a mistake
Comment #18
jibranAddressed #17 and some PHPCS issues.
Comment #19
borisson_#18 correctly resolves #17.
Comment #20
catchLooks like a good change, just a couple of nitpicks:
Shouldn't this be form (singular)?
AJAX.
.:
Comment #21
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedadded patch as per #20
Comment #22
jibran#20 is addressed so back to RTBC.
Comment #23
dpiPlugins can support multiple, though actions support one at this time.
* Get the action plugin while ensuring it implement configuration form.
English
Comment #24
jibranFixed the English.
Comment #26
jibranFails seem unrelated.
Comment #28
jibranComment #30
jibranComment #32
jibranComment #33
larowlanAjax forms work without JavaScript. Can we make this a BTB instead, cause ::waitForElement is brittle.
Would require changing the test form to use something like a submit button instead.
Comment #34
larowlansaving review credits
Comment #35
jibranPersonally, I think this problem is bigger than this issue and we are now moving away from gaston JS to the webDriver, hopefully, that will end our random fail issue.
For this specific issue, adding JTB test is essential. I have discovered this issue while using #2797583-48: Dynamically provide action plugins for every moderation state change patch via UI. As you can see the patch from the linked comment is green, even in
Drupal\Tests\content_moderation\Functional\ActionConfigurationTest
we are submitting the action form. I think converting JTB test to BTB will lose the functionality we want to check here.Based on the reason given above I'm changing status from NR to RTBC if you still think it's still not enough then feel free to set it back to NW.
Comment #36
jibranComment #38
jibranComment #40
larowlanCan we get a followup to have ActionBase implement PluginFormInterface with empty implementations, and then remove this.
duck-typing++
Committed asad1e35c and pushed to 8.5.x.
Thanks