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:

  1. 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 its buildConfigurationForm() so we have a case to work on.
  2. Go to /admin/config/system/actions and add "Send email" action.
  3. 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)
  4. Submit the form
  5. Now go to edit again this freshly created configurable action.
  6. Expected: to see all the values you've submitted
  7. 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).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bucefal91 created an issue. See original summary.

bucefal91’s picture

Status: Active » Needs review
FileSize
2.28 KB

As 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.

  public function buildForm(array $form, FormStateInterface $form_state) {
    $this->plugin = $this->entity->getPlugin();
    return parent::buildForm($form, $form_state);
  }

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpi’s picture

Version: 8.4.x-dev » 8.5.x-dev
Assigned: Unassigned » dpi

Looks like this is still an issue.

dpi’s picture

dpi’s picture

Priority: Normal » Major

The last submitted patch, 6: 2816861-action-plugin-ajax-6-testsonly.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dpi’s picture

Test only patch failed successfully.

Updating patch with coding standards.

Status: Needs review » Needs work

The last submitted patch, 9: 2816861-action-plugin-ajax-9.patch, failed testing. View results

dpi’s picture

This should work

dpi’s picture

Status: Needs work » Needs review
jibran’s picture

This is blocking #2797583: Dynamically provide action plugins for every moderation state change now.

+++ b/core/modules/action/src/ActionFormBase.php
@@ -50,7 +43,6 @@ abstract class ActionFormBase extends EntityForm {
-    $this->plugin = $this->entity->getPlugin();

@@ -85,8 +77,8 @@ abstract class ActionFormBase extends EntityForm {
+    if ($this->entity->getPlugin() instanceof PluginFormInterface) {
+      $form += $this->entity->getPlugin()->buildConfigurationForm($form, $form_state);

@@ -121,8 +113,8 @@ abstract class ActionFormBase extends EntityForm {
+    if ($this->entity->getPlugin() instanceof PluginFormInterface) {
+      $this->entity->getPlugin()->validateConfigurationForm($form, $form_state);

@@ -132,8 +124,8 @@ abstract class ActionFormBase extends EntityForm {
+    if ($this->entity->getPlugin() instanceof PluginFormInterface) {
+      $this->entity->getPlugin()->submitConfigurationForm($form, $form_state);

Let's create a helper method for this.

dpi’s picture

@ #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.

jibran’s picture

Thanks for addressing the feedback. This is ready.

+++ b/core/modules/action/tests/src/FunctionalJavascript/ActionFormAjaxTest.php
@@ -0,0 +1,70 @@
+  ¶

It can be fixed on commit.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
+    if ($plugin = $this->getPlugin()) {
+      $form += $plugin->buildConfigurationForm($form, $form_state);
+      $plugin->submitConfigurationForm($form, $form_state);

This seems like a mistake

jibran’s picture

Status: Needs work » Needs review
FileSize
2.16 KB
9.35 KB

Addressed #17 and some PHPCS issues.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

#18 correctly resolves #17.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Looks like a good change, just a couple of nitpicks:

  1. +++ b/core/modules/action/src/ActionFormBase.php
    @@ -147,4 +137,17 @@ public function save(array $form, FormStateInterface $form_state) {
    +   * Gets the action plugin while ensuring it implements configuration forms.
    

    Shouldn't this be form (singular)?

  2. +++ b/core/modules/action/tests/action_form_ajax_test/src/Plugin/Action/ActionAjaxTest.php
    @@ -0,0 +1,89 @@
    + * Plugin used for testing ajax in action config entity forms.
    

    AJAX.

  3. +++ b/core/modules/action/tests/src/FunctionalJavascript/ActionFormAjaxTest.php
    @@ -0,0 +1,70 @@
    +    // Configuration should be shown in edit form.:
    

    .:

harsha012’s picture

Status: Needs work » Needs review
FileSize
9.42 KB
1.91 KB

added patch as per #20

jibran’s picture

Status: Needs review » Reviewed & tested by the community

#20 is addressed so back to RTBC.

dpi’s picture

Assigned: dpi » Unassigned
Status: Reviewed & tested by the community » Needs work

Shouldn't this be form (singular)?

Plugins can support multiple, though actions support one at this time.

* Get the action plugin while ensuring it implement configuration form.

English

jibran’s picture

Fixed the English.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2816861-action-plugin-ajax-18.patch, failed testing. View results

jibran’s picture

Status: Needs work » Reviewed & tested by the community

Fails seem unrelated.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2816861-action-plugin-ajax-18.patch, failed testing. View results

jibran’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2816861-action-plugin-ajax-18.patch, failed testing. View results

jibran’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2816861-action-plugin-ajax-18.patch, failed testing. View results

jibran’s picture

Status: Needs work » Reviewed & tested by the community
larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/action/tests/src/FunctionalJavascript/ActionFormAjaxTest.php
@@ -0,0 +1,70 @@
+class ActionFormAjaxTest extends JavascriptTestBase {
...
+    $this->assertSession()->waitForElement('css', '[name="party_time"]');

Ajax 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.

larowlan’s picture

saving review credits

jibran’s picture

Ajax forms work without JavaScript. Can we make this a BTB instead, cause ::waitForElement is brittle.

Personally, 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.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: 2816861-action-plugin-ajax-18.patch, failed testing. View results

jibran’s picture

Status: Needs work » Reviewed & tested by the community

  • larowlan committed ad1e35c on 8.5.x
    Issue #2816861 by dpi, jibran, bucefal91, harsha012, tim.plunkett, catch...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/modules/action/src/ActionFormBase.php
@@ -147,4 +137,17 @@ public function save(array $form, FormStateInterface $form_state) {
+  protected function getPlugin() {
+    if ($this->entity->getPlugin() instanceof PluginFormInterface) {
+      return $this->entity->getPlugin();
+    }
+    return NULL;
+  }

Can 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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.