Element plugin object wrappers causing AJAX failures

catch → credited graber → .

Crediting @graber who actually found the bug and bisected it - we were discussing in slack.

So far found out that for some reason on AJAX call the resulting textfield element '#value' is set to an empty string after the change which prevents FormBuilder::handleInputElement() to set its value from user input.

Root cause is that in FormBuilder::processForm(), $form is copied to $unprocessed_form before the call to buildForm() maps user input to #value properties. The value of $unprocessed_form is what is cached at the end of processForm():

    if (!$form_state->isRebuilding() && $form_state->isCached()) {
      $this->setCache($form['#build_id'], $unprocessed_form, $form_state);
    }

So when the form is retrieved from cache, such as in an AJAX submit, the form element should not have #value set, so in handleInputElement(), (!isset($element['#value']) && !array_key_exists('#value', $element)) evaluates to TRUE, and $element['#value'] can be set to the input value.

With the change to ElementInterface objects, after $form is copied to $unprocessed_form, elements between the two stay coupled, because of use of objects and references to arrays within the render array. So before $unprocessed_form is saved to cache, elements within the array have been changed in sync with $form, and $element['#value'] throughout elements in the form is set. Then when the form is retrieved from cache, the user input can not populate $element['#value'], since it is already set.

I've tried a few different methods to decouple $unprocessed_form from $form, including just serializing the form (see MR 12899), but nothing's worked so far.

Do we just need to clone it?

We need a helper on element info manager to copy the object correctly for this situation.
I'd tried something along the lines of $unprocessed_form = (clone $this->elementInfo->fromRenderable($form))->toRenderable();, and that didn't work, since I think the copy is too shallow.

Then I tried implementing __clone in RenderElementBase to recursively clone all the element objects, and I think I hit a stack overflow.

I also tried NestedArray::filter($form, static fn ($element) => !($element instanceof ElementInterface), and that OOM'd.

I just pushed up a commit to handle the serialize exception. This change makes the LMS test pass, as least locally, but it's not passing here.

I think the next step is to try to create a failing core test. LMS is interesting in that it has AJAX modals stacked on top of each other in the entity forms, so maybe there's something happening because of that that's not covered in even core media library tests. I haven't tracked exactly how/why the unprocessed form array is coupled to the original array. I just saw in the debugger that properties had changed after the call to doBuildForm($form), and hopefully a (simpler) core test can make it easier to see why that's happening.

OK, so what I think is special about the LMS form is that it is opened in a modal via AJAX POST request, and the initial build of the form is cached. For most forms, the initial build is on a GET request that can not be cached. This causes the issue with the unprocessed form being cached incorrectly.

I remembered I wrote a kernel test for 🐛 Form state storage changes in #process callbacks are not cached during form rebuild. Needs work that also addresses weird form caching errors, so I borrowed some of that to write a kernel test that I believe simulates what's going on and makes it easier to debug. I pushed up a commit with the new kernel test, and you can see that the test only job fails, but the "fix" with serializing the unprocessed form makes the test pass.

Given that it's somewhat unusual for the initial build of a form to be on a POST request, I think the priority can be bumped down to Major, but leaving it for now.

Might be enough to do this:

public function copy($form) {
  $copy = $form; 
  unset($copy['##object'];
  return $copy;
}

We may need to pass form in by reference and unset ##object on form too, I can test when I get to a computer.

Ok I had a chance to push it up so we can test LMS (which issue did you test on)?

Your test still doesn't pass this, I tried unsetting object on the form by reference before the copy too, but that didn't work either for your test.

I don't know if it'll solve the issue completely for LMS but not caching the form on initial build is going to be a performance improvement in any case. I'll try it now.

Ok, it doesn't solve the issue in all cases because there are other situations where a modal form should get cached before final submit (failed validation, AJAX widgets like adding field items or file uploads, ..).

3539320-element-plugin-object branch - fixes the LMS issue
3539320-copy branch - empty string textfield `#value` is still cached.

Implemented dereferencing of children in the copy MR, this fixes LMS issues.
One question: since it wasn't about the `##object` values of each element but about certain form elements being references and therefore altered as memory addresses at their source by other code.. don't we actually have a different root cause of now altering more than we altered before?
The solution works also when removed the line that unsets `$copy['##object']`: https://git.drupalcode.org/project/drupal/-/merge_requests/12901/diffs?c...
so that being there wasn't the issue.

One question: since it wasn't about the `##object` values of each element but about certain form elements being references and therefore altered as memory addresses at their source by other code.. don't we actually have a different root cause of now altering more than we altered before?
The ##object properties don't affect anything per se here, but we need to unset them because they are referencing the wrong place and could conceivably present problems in other ways.

When we use the element objects to build a render array , it can ends up something looking like this:

array $build = [
  #type => some type,
  ##object => Element object 1,
  child => ref [
    #type => another type
    ##object => Element object 2,
    ...
  ]
  ...
]

When the array is copied to a new variable $copy because of the references, it looks the same

array $copy = [
  #type => some type,
  ##object => Element object 1,
  child => ref [
    #type => another type
    ##object => Element object 2,
    ...
  ]
  ...
]

So we want to break those array references and object references, because without doing that, if you called ElementInfoManager::fromRenderable($copy['child']), this would affect Element object 2, and in turn, possibly the original $build array as well. Will probably need test coverage for this...

array $copy = [
  #type => some type,
  child => (not a reference) [
    #type => another type
    ...
  ]
  ...
]

This probably also means we need to keep an eye on anywhere in code where render arrays are copied...

I push up a commit to MR 12899. Looks like @nicxvan and I ended up in similar places.

Tests pass now, but I think there might need to be more test coverage per #18, and probably a CR. Will think about that later because looking at all this have given me a headache for now.

I pushed another version that doesn't require copying or cloning or needing to know how to specially copy form or a render element.

This doesn't pass the test, but it is an interesting direction.

When @nicxvan mentioned ‘initializeInternalStorage’ on Slack I thought that we could save an unprocessed version that wouldn’t be a reference then. Something like ‘$this->unprocessed = $element’ (no reference) and use that when getting the unprocessed form.

12899 and 12901 I think share the same flaw which is that you need to know to use the special copy function where before you could just copy the array.

I think 12907 solves this, but in possibly the worst possible way cause now it copies every single time.

It should pass though and maybe we can improve it now that we see what is going on.

Maybe a deep copy for the form is better and needing to copy the form and render arrays with a special function is ok, but that feels like bad DX.

Tagging for 11.3.0 release target. Wondering if we should revert 📌 Slowly, very slowly start OOPifying the render system Needs review then recommit it again with the tests + fix added here.

Updated 12899 with an additional test for shallow vs deep copies of these render arrays, as well as element object cloning. I think the tests show that we probably can't avoid adding a recursive copy method, though I don't love this. While people aren't using the element objects to create render arrays yet, when they do, having copies of render arrays entangled with each other because of references will probably lead a lot of unexpected behavior.

Alternatively we'd probably need to avoid using references in methods like initializeInternalStorage() and addChild(), but doing that probably causes problems of its own.

Will add a CR for the deep copy if we're good with the 12899 approach.

Is there a way to trigger an error on a shallow copy and require a deep copy?

We wouldn't be able to do this bc break in a minor I think and I think we need to deeply consider the dx.

People are used to being able to just duplicate and I think it's clear we're breaking that.

Maybe duplicate is a better verb for the method?

Is there a way to trigger an error on a shallow copy and require a deep copy?

No, it's just PHP assignment $array1 = $array2; and there's no way to hook into that.

People are used to being able to just duplicate and I think it's clear we're breaking that.

Agreed, but there aren't too many options. We could revert it all for now, and make it a major-only change(?), but that might not be better.

On one hand, it's not all render arrays that are affected, just ones built from element objects. On the other hand, the relevant arrays are content entity forms with field widgets.

Maybe duplicate is a better verb for the method?

I'm not opinionated on the terminology, but NestedArray has the mergeDeep() and mergeDeepArray() methods which is where I got it from.

Couple more thoughts:

Another alternative is to stop using references as I mentioned in #26. Every array would just be copied into and out of the element objects, and there would be no ##object properties. This would mean the element objects would need to be instantiated new every time you wanted to convert an array into one. I also have slight concerns wrt memory usage when copying what could be large render arrays back and forth.

Also, at some point in the future when the arrays are completely in the past and we're only using objects, we'll have to get used to using shallow vs deep copies, so if there's some solution for that we can come up with now, all the better.

When eventually render objects will be actually rendered, the only way to have the unprocessed form will be deep cloning of the form object so 12899++.
Instead of doing what's been done in 📌 Slowly, very slowly start OOPifying the render system Needs review , I'd start from supporting rendering of those objects in some ObjectRenderer that would be used in the current renderer conditionally if an object is passed to the render() method - creating new API next to the existing one at the very bottom of the system and only then switching gradually and not implementing some mixed array/object things in the middle of the processing chain so also ++ for reverting 📌 Slowly, very slowly start OOPifying the render system Needs review and rethinking the entire concept.

📌 Slowly, very slowly start OOPifying the render system Needs review just got reverted so we should move this discussion back over there. Don't want to mark this as duplicate just yet because it'll drop it out of everyone's 'my issues' but would be good to move the test coverage back into that issue and maybe the latest one or two proposed fixes in separate draft MRs or similar.