Non-array values for #ajax

MR in #20 also fixes

Error: Cannot use a scalar value as an array in Drupal\Core\Render\Element\RenderElement::preRenderAjaxForm() (line 315 of /<redacted>/web/core/lib/Drupal/Core/Render/Element/RenderElement.php)

when adding a custom entity reference field (PHP 8.1, Drupal 10.0.8).

I added the documentation from #18. Please review.

- updated summary
- add related issue

Also drupal.org and api.drupal.org documentations should be updated.

Exactly same as MR. To run against 11.x

Passed 11.x also

Reviewing the changes seem fine. MAY need a test but will let committer decide.

The last submitted patch, 29: 2651418-29.patch, failed testing. View results →

run re-test ... pass ... "revert the issue status"

Evaluating need for tests against https://www.drupal.org/project/drupal/issues/2972776#heuristics 🌱 [policy, no patch] Better scoping for bug fix test coverage RTBC (Which is not yet adopted, just as a thought exercise).

1. Are there clear steps to reproduce on the issue?Yes
2. Is it easy to verify via manual testing that the bug is fixed?Yes
3. Is it a 'trivial' patch with small, easy to understand changes?Yes
4. Is the code being changed in self-contained/@internal code that we don't expect contrib modules to interact with significantly? (plugins, controllers etc.)Yes
5. In the case that the bug is committed with test coverage then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed (for example site were unlikely to be actively mitigating the bug with workarounds/patches that would be removed when it's fixed)?Yes
6. Is the bug fix achieved without adding new, untested, code paths?Yes
7. To add test coverage, would it need an explicit 'regression' test that mainly prevents us reverting the patch rather than improving coverage in general?Yes
8. If the test coverage was deferrred to a follow-up, would it be easy for someone who didn't work on the original bug report to pick it up?Yes
9. Does the issue expose a general lack of test coverage for the specific subsystem, and if so would it be better to add generic test coverage for that subsystem in a separate issue so that the test coverage can be added in a maintainable way, rather than a regression test?Yes

So on that basis I think this is a good candidate for not adding tests, but I'll ask for second opinion

+++ b/core/lib/Drupal/Core/Render/Element/Ajax.php
@@ -5,7 +5,21 @@
+ * set to true to use default values.

nit I think this should be TRUE not true, can be fixed on commit

Moving to 10.1.x as 10.0.x is now security only

larowlan → credited lauriii → .

+++ b/core/lib/Drupal/Core/Render/Element/RenderElement.php
@@ -268,6 +268,11 @@ public static function preRenderAjaxForm($element) {
+    // If set to TRUE, default values should be applied.
+    if ($element['#ajax'] === TRUE) {
+      $element['#ajax'] = [];
+    }

Is this actually needed?

We have instances of this in core in selection handlers, and they work - so I suspect not?

If they work without this change, this is just a docs issue and therefore we don't need tests

Crediting some others who helped me to review this

@larowlan as core introduces '#ajax' => TRUE, in several places for some elements
And as discussions on the issue and also on related issues say,
i would say we need to make sure it doesn't bring issues with it.

I updated the MR adding just some small tests ... about element wit no ajax but also with no empty AJAX ( '#ajax' => TRUE,)
And without this fix ... it will fail.

On #33:

Is the code being changed in self-contained/@internal code that we don't expect contrib modules to interact with significantly? (plugins, controllers etc.)Yes

This should be a NO - the changes are in the render/AJAX system and the bug can be triggered by contrib usage of the API. Maybe we should re-word that one to make it more explicit?

To add test coverage, would it need an explicit 'regression' test that mainly prevents us reverting the patch rather than improving coverage in general?Yes

There is RenderElementTest so it might be a case of adding a method here, I'm not sure how straightforward that is though so probably a maybe?

Its sounds like #36 added tests, so this one is ready for review again I think

Can confirm #36 that without the fix the additional tests do fail.

Error : Cannot use a scalar value as an array

Posted comment to the MR

Moved the new docs to core.api.php.

Hiding patches if fix is going to be in MR.

MR needs to be updated to 11.x

hmm ... this issue seems to be for 10.1.x ... not 11.x

but this still means we need to update the target branch .. so is there anyone with the permissions to update the target source

or i could create new MR(s) ... for 10.1.x and 11.x?

I rebased against 11.x.

Thanks! Happy to see that update the target branch didn't cause much issue, know sometimes it does.

Looking at the changes, the comments seem fine

Running test-only feature I get

There was 1 error:
1) Drupal\Tests\Core\Render\Element\RenderElementTest::testPreRenderAjaxForm
Error: Cannot use a scalar value as an array
/builds/issue/drupal-2651418/core/lib/Drupal/Core/Render/Element/RenderElement.php:322
/builds/issue/drupal-2651418/core/tests/Drupal/Tests/Core/Render/Element/RenderElementTest.php:63
/builds/issue/drupal-2651418/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-2651418/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-2651418/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-2651418/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-2651418/vendor/phpunit/phpunit/src/TextUI/Command.php:97
ERRORS!

Think this is good

So the reason ['#ajax'] = TRUE works in the entity reference items is because it has special code that changes it into an array. It is not meaning to change it to default values at all :)

See \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::fieldSettingsAjaxProcessElement() for where this happens. If this did not occur then treating TRUE like an array would produce errors on PHP 8.1.

I think we could consider not making this change but instead documenting the places in the entity reference system where #ajax is being set to TRUE with an

@see \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::fieldSettingsAjaxProcessElement() 

@alexpott this is not related with

\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem

But "others" as
\Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection
(Maybe also with) \Drupal\user\Plugin\EntityReferenceSelection\UserSelection

Details about the issue/errors are in the comments and also related issue
Also updated the issue info.

Could you please check/review again?
Thanks

@vasike I promise you this has everything to do with EntityReferenceItem::fieldSettingsAjaxProcessElement().

The Plugin\EntityReferenceSelection\DefaultSelection::buildConfigurationForm() and Plugin\EntityReferenceSelection\UserSelection::buildConfigurationForm() are called by \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::fieldSettingsForm() - specifically from the line:

  $form['handler']['handler_settings'] += $handler->buildConfigurationForm([], $form_state);

The ::fieldSettingsForm() method is the one that adds '#process' => [[static::class, 'fieldSettingsAjaxProcess']], too... which results in

      $element['#ajax'] = TRUE;

being converted to

      $element['#ajax'] = [
        'trigger_as' => ['name' => 'handler_settings_submit'],
        'wrapper' => 'field-combined',
        'element' => $main_form['#array_parents'],
      ];

This #ajax => TRUE is just a shorthand for Entity selection handlers - so that they can get the correct [‘#ajax’][‘element’] set without having to repeat the #process logic everywhere.

Thanks a ton @alexpott for taking a look!

New MR https://git.drupalcode.org/project/drupal/-/merge_requests/6458
and new approach - make sure we have an array for #ajax element property ... get rid of '#ajax' => TRUE

And it seems it works (All Green)
Also checked with the related issue MR ... and we don't get the errors.

What do you think @alexpott?

And of course thank you.

smustgrave → changed the visibility of the branch 11.x to hidden.

smustgrave → changed the visibility of the branch 2651418-non-array-values-for to hidden.

Possible to get a test for the new approach?

The current approach would be a BC break since before a value of FALSE would not add ajax but now it would. Also a value of TRUE would be not add the ajax default so custom and contrib would be broken.

I don't think it is worth changing the behaviour here. I think it is worth added @see's to where #ajax is being set to TRUE so we don't wonder the whys and wherefores in the future.

ok. there could BC breaks
small update for TRUE cases.

imho, there could a change records to invite custom devs and contrib maintainers to use [] instead of TRUE.
To avoid some cases as described in the issue and the "use case" from related issue.

@vasike I do not understand why we want to change this. We can add the @see's so people are not confused in the future and be done.

@alexpott thanks a lot for your persistence: I was blind, but not ... I can see
Sorry I didn't listen ... I was blinded by the "edge case" in Views Modal ... which it doesn't work ("properly") with form validation or ajax ... but this is another story/issue (Drupal chapter).

So, for now 2 updates:
1. new "documentation" MR
2. Update the related issue MR ... removing the ajax, as we did for #required property
within this commit https://git.drupalcode.org/project/drupal/-/merge_requests/4053/diffs?co...

How are we ... now?

p.s. Maybe in another places, this could work
$form['#process'][] = [EntityReferenceItem::class, 'fieldSettingsAjaxProcess'];
But do we need to mention ... somewhere?

vasike → changed the visibility of the branch 2651418-ajax-array-only to hidden.

Thanks @alexpott for taking another look.

MR threads addressed.

Believe the feedback from @alexpott has been addressed here but left 1 comment about one of the comments.

indeed ... missed to include the change in the previous commit

Thanks for addressing!

Backported to 10.2.x as this is a docs fix.

Committed and pushed fe3aa496f7 to 11.x and 2404bcd91a to 10.3.x and b825f8e2e5 to 10.2.x. Thanks!

For those like @DieterHolvoet who experience bugs due to this - it must be happening because your render array contains an unprocessed form. Forms must be processed before the prerender is called so toy need to track down why that is occurring.

quietone → closed merge request !6726

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

vasike → closed merge request !6458