Problem/Motivation
Our documentation specifies that #ajax
items in render arrays must be arrays. RenderElement::preRenderAjaxForm()
confirms this behavior. Yet in three places we set '#ajax' => TRUE
in render arrays.
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)
Steps to reproduce
Run MR's test without a fix.
Or try related issue's MR without this change/fix https://git.drupalcode.org/project/drupal/-/merge_requests/4053/diffs#01...
// Workaround for https://www.drupal.org/project/drupal/issues/2651418.
// @todo Remove the below when the referenced issue is fixed.
foreach (Element::children($sub_handler_settings) as $key) {
if (\array_key_exists('#ajax', $sub_handler_settings[$key]) &&
!\is_array($sub_handler_settings[$key]['#ajax'])
) {
$sub_handler_settings[$key]['#ajax'] = [];
}
foreach (Element::children($sub_handler_settings[$key]) as $sub_key) {
if (\array_key_exists('#ajax', $sub_handler_settings[$key][$sub_key]) &&
!\is_array($sub_handler_settings[$key][$sub_key]['#ajax'])
) {
$sub_handler_settings[$key][$sub_key]['#ajax'] = [];
}
}
}
Proposed resolution
- Update \Drupal\Core\Render\Element\Ajax documenation.
- Make sure '#ajax' => TRUE
are accepted - transform into array in core/lib/Drupal/Core/Render/Element/RenderElement.php
Remaining tasks
- Update drupal.org and api.drupal.org documentations after commit.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|
Issue fork drupal-2651418
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
XanoComment #4
dawehnerAre we sure we don't need any kind of manual testing for it?
Comment #5
XanoI first wanted to see if these items did not actually do anything, but the test failures seem to indicate they do. I'd need to analyze the test to say anything useful.
Comment #6
tim.plunkettSo #ajax => TRUE is shorthand for "I want all of the default values".
In \Drupal\Core\Render\Element\RenderElement::preRenderAjaxForm(), we first check
if (empty($element['#ajax'])) {
So an empty array wouldn't work there.
Then later we have
if (isset($element['#ajax']) && !isset($element['#ajax']['event'])) {
And it derives the
$element['#ajax']['event']
and other values based on$element['#type']
So we just need to fix our docs.
Comment #7
XanoWhere in the code do we explicitly allow
TRUE
? The first line inRenderElement::preRenderAjaxForm()
that accesses specific#ajax
contents isif (!empty($element['#ajax']['disable-refocus'])) {
, which does not fail if#ajax
, but before this there's no code that checks for boolean values either.Then further down, in the switch statement,
#ajax
array items are written to, but at that point#ajax
is not confirmed to be an array yet. This went wrong when I was testing https://github.com/bartfeenstra/drupal-verf, because line 316 inRenderElement::preRenderAjaxForm()
was reached, which is$element['#ajax']['event'] = 'change';
, even though$element['#ajax'] == TRUE
, which comes fromDefaultSelection
.Comment #16
larowlanDoes this issue still apply?
If so, can someone update the issue to use the new issue summary template.
Judging by the last comments, it looks like this is a documentation issue, however there's come conjecture about whether that actually works.
I found some instances of this in modules I maintain and can confirm they work, so that feels like it is a docs issue.
Comment #18
quietone CreditAttribution: quietone at PreviousNext commentedHere is an attempt to add documentation by someone who doesn't know render arrays and #ajax. Even with that I reckon it will be easier for others to comment if there is something here to start with. Not running tests because this is only documentation.
Comment #21
DieterHolvoet CreditAttribution: DieterHolvoet as a volunteer commentedI'm encountering this issue when adding an entity reference views filter as being worked on in #2429699: Add Views EntityReference filter to be available for all entity reference fields. The stack trace:
I opened a MR fixing the issue.
Comment #22
a.dmitriiev CreditAttribution: a.dmitriiev as a volunteer and at 1xINTERNET commentedI have the same problem as DieterHolvoet . I am also using patch from #2429699: Add Views EntityReference filter to be available for all entity reference fields issue. MR fixes the problem. It seems that tests are failing for some infrastructure reason.
Comment #23
catchThe MR is missing the documentation change from the patch in #18, looks to me like we need both. Could also still use an issue summary update.
Comment #24
Graber CreditAttribution: Graber at Tag1 Consulting for American Federation of Teachers commentedWhile here, I'd also change that '#ajax' to array in /lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php.
Comment #25
rodrigo panchiniak fernandes CreditAttribution: rodrigo panchiniak fernandes at European Commission and European Union Institutions, Agencies and Bodies commentedMR in #20 also fixes
when adding a custom entity reference field (PHP 8.1, Drupal 10.0.8).
Comment #27
rpayanmI added the documentation from #18. Please review.
Comment #28
vasike- updated summary
- add related issue
Also drupal.org and api.drupal.org documentations should be updated.
Comment #29
smustgrave CreditAttribution: smustgrave at Mobomo commentedExactly same as MR. To run against 11.x
Comment #30
smustgrave CreditAttribution: smustgrave at Mobomo commentedPassed 11.x also
Reviewing the changes seem fine. MAY need a test but will let committer decide.
Comment #32
vasikerun re-test ... pass ... "revert the issue status"
Comment #33
larowlanEvaluating need for tests against https://www.drupal.org/project/drupal/issues/2972776#heuristics (Which is not yet adopted, just as a thought exercise).
So on that basis I think this is a good candidate for not adding tests, but I'll ask for second opinion
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
Comment #35
larowlanIs 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
Comment #36
vasike@larowlan as core introduces
'#ajax' => TRUE,
in several places for some elementsAnd 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.
Comment #37
catchOn #33:
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?
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?Comment #38
larowlanIts sounds like #36 added tests, so this one is ready for review again I think
Comment #39
smustgrave CreditAttribution: smustgrave at Mobomo commentedCan confirm #36 that without the fix the additional tests do fail.
Error : Cannot use a scalar value as an array
Comment #40
lauriiiPosted comment to the MR
Comment #41
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedMoved the new docs to core.api.php.
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedHiding patches if fix is going to be in MR.
MR needs to be updated to 11.x
Comment #43
vasikehmm ... 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?
Comment #44
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedComment #45
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedI rebased against 11.x.
Comment #46
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks! 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
Think this is good
Comment #47
alexpottSo 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
Comment #48
vasike@alexpott this is not related with
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
Comment #49
alexpott@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:
The ::fieldSettingsForm() method is the one that adds
'#process' => [[static::class, 'fieldSettingsAjaxProcess']],
too... which results inbeing converted to
Comment #50
alexpottThis
#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.Comment #51
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks a ton @alexpott for taking a look!
Comment #53
vasikeNew 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.
Comment #56
smustgrave CreditAttribution: smustgrave at Mobomo commentedPossible to get a test for the new approach?
Comment #57
alexpottThe 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.
Comment #58
vasikeok. 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 ofTRUE
.To avoid some cases as described in the issue and the "use case" from related issue.
Comment #59
alexpott@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.
Comment #61
vasike@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
propertywithin 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?
Comment #63
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks @alexpott for taking another look.
Comment #64
vasikeMR threads addressed.
Comment #65
smustgrave CreditAttribution: smustgrave at Mobomo commentedBelieve the feedback from @alexpott has been addressed here but left 1 comment about one of the comments.
Comment #66
vasikeindeed ... missed to include the change in the previous commit
Comment #67
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks for addressing!
Comment #68
alexpottBackported 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.