Add test coverage ensuring user input is mapped on the correct form elements when elements are reordered

From the issue summary:

A test field with two properties and a widget for this field where the one property is for user input and the other one is of type hidden.

I would suggest using the existing ShapeItem (test) field type and just add a widget for it :)

Thank you @hchonov, the latest test is very readable and the failing test precisely shows the problem with updating the stored entity: before it was the circle that was blue, and now it is the rectangle. Thanks you also for making this a separate issue. That way we can commit this test stand-alone and make sure we never break this regardless of what we end up doing in the other issues.

For me this is RTBC, but @Berdir should definitely have a look at this, because he was (not necessarily advocating but at least) experimenting with updating the stored entity in light of the recent issues.

Also adding some related issues.

+1 on not updating the form entity in the validation phase, since the original entity is supposed to be a valid entity and the submitted one may not be.

Looked at the patch again and read through the issue summary, both are very clear and describe the issue well. I think it's a good idea to start chucking away at some of the problems in this space in small steps, and this is a very nice simple step.

@Berdir can still take a look at this, but marking RTBC for now, this really does seem really straightforward.

Ideally we would stop adding web tests.

Good point!

Luckily, that was not very hard to convert. Also cleaned up a couple of minor things. (The class rename is not in the interdiff, sorry.)

Tagging as well as the @berdir assignment so it's easier to scan what's happening. Great idea splitting this out.

Ok, so i finally found some time to debug this and trying to better understand that.

I see what the test is trying to do but I remain unconvinced about the sorrounding argument/conclusion, at the very least, it makes it sound much simpler than it is in reality:

This only fails because the hidden element uses #value. It passes if you use #default_value for the hidden element. If what the issue summary says is true then that would also mean that the color doesn't change, but that's not the case. The difference is that it fails because we *do not* set the value that was provided by the user, by using #value, we ignore the actual user input. The color field on the other side is that on a rebuild, the form system will actually consider the original user input and not the default_value. That's why it works in the first place.

That brings me to the second point, "content entity form has to be rebuilt with the original unmodified entity". That's not true. We already *are* mixing the original with user input, just in a not so obvious way. For example, "Add another item". What happens there is actually quite interesting:
Lets say we have a multi-value text field and you are editing an entity that has one value already. The form adds a second one for you. It also ensures that the field items object has enough deltas in \Drupal\Core\Field\WidgetBase::formMultipleElements(), by calling $items->appendItem(); So at this point, we already altered our entity object (as $items is by reference). Now you fill out the second form element, and press Add another item. Then the original entity now has 3 deltas, and we build two empty form elements, relying on the form system to fill it with the user input that the user provided. IMHO, this is pretty weird and feels very counter-intuitive.

It IMHO also means that we built a test that isn't too close to reality, as we now specifically work against the user input, which we otherwise rely on but then still allow that to be mapped back to the entity. What would happen if you actually try to add a third item, lets say a red circle? Even if you try to add some logic that says circles are by default red, it will not be possible to get that value into the form.

The thing with the weight re-ordering is that the field UI has a lot of logic to work with the fact that we are building a pretty weird form partially based on the original entity. It has logic to remember the original delta which it uses to map back the validation errors (which are actually using the new deltas).

Note how content entity forms are different than config entity forms (#2813073: Don't update $this->entity in EntityForm::afterBuild()) those already always rebuild the entity on every form rebuild. And some of them are pretty complicated too, they just handle it differently. Specifically, in situations like this, what you do is the opposite of what field widgets do. You'd unset the user input so that the form will be displayed using your default_values and not the previous delta. Then we would actually *need* the updated entity. I can imagine that this would make certain widgets (like the insanely complicated file widget) and validation handling a lot easier.

Ok, that was pretty long. Here's my main point:

"content entity form has to be rebuilt with the original unmodified entity" is the wrong goal IMHO. The goal is to have an editing experience that works correctly for all cases. Whether that means to rebuild with the original entity or actually the opposite and change how field re-ordering works remains to be seen.

Not exactly sure about the next step. Can we figure out a widget that is closer to a real thing and actually allows to add new items and still has a problem?

That's a needs review, at least. Need some time to think before i post a proper reply.

#value exists, but it makes little sense in most cases. Usually that's when you use #type value instead. But of course it would have the same problem.

I don't really see a security problem with using #default_value, after all, the user *did* select/create that entity and validation/access will still run anyway.

To be clear, I'm OK with adding a test like this. I'm just trying to better understand it and as I wrote, I don't see how you can actually *add* a new item with a widget like this. Any chance you can share your actual widget?

Hm. This isn't actual user input, but fair enough, this could be done in a similar way if you'd store the value in widget state.

However, now something fun is happening that I think contradicts your argument from above.

This is exactly true, that "content entity form has to be rebuilt with the original unmodified entity". What you are pointing to is something completely different because the entity is modified during the form building process based on the form state, but is not stored with this changes in the form state and the next time the form is rebuild the form is rebuild with the unmodified entity and it just is modified during the form building process. This are two different things.

If you run this test with a breakpoint in FormCache::getCache()/FormCache::setCache(), you will notice that the entity there now contains 4 items and it includes two red circles. That means both adding items *and* your change are in fact persisted in the entity that is put in form cache.

$items are object and they are by reference, so anything you do in your widget to the entity and its fields *is* cached.

My point is that this is not as black and white as you try to make it sound. We are *definitely* altering the entity to be able to build the form out correctly. Multiple values is a relatively easy case for that, we can just fake empty items and rely on the user input to have them be filled out, but take something like FileWidget which actually needs to display the selected image, with completely different markup, buttons and what not? It explicitly works around this fact by storing the values in widget state and then calls $items->setValue(). And based on what we've seen above, this *will* be stored on the entity in the form state cache. And exactly that workaround, which is definitely required, is for example hurting us again in #2548713: Only one additional new value saved unlimited field and no non-field values are restored after preview because we have to make changes there that are possibly API breaking. If $entity/items would just contain the user input, most use cases for widget state would simply go away and things would just work.

And that's a single form element. What about nested paragraphs, where we have to build whole entity forms based on the input provided by the user? Paragraphs *needs* the build entities to know what to build. And to build the entities, we need the form elements.

Back to multiple values and re-ordering. The reason it is so complicated in the first place is because we are mixing so many things together that it's not even funny anymore. We have the partially altered "original" entity, we have the widget state to make further changes to it, we then lay the existing user input over it and to make it look like we re-ordered it, we sort it based on the user input in template_preprocess_field_multiple_value_form() so that the order of elements is then not how the deltas are in the form elements but the weights.

"content entity form has to be rebuilt with the original unmodified entity" is why it has to be so complicated. It is not the solution to all our problems. One could even argue that it is *the problem* :)

If $items would be what the user submitted, we'd simply drop the user input for that part of the form (something that is common in plain and config entity forms) and rebuild the form elements based on the current entity values. Done. (There might be other problems with that approach, hard to say without trying it out)

This means that what the test is doing is correct with the current core form building and this test has to pass no matter how we deal with the entites.

Yes, that is true. After discussing this and actually learning and re-learning all the things above (I did not actually know all that, I put it together while debugging and writing those comments), I'm perfectly fine with the test like it is now. I understand now how it could work in a real use case, in combination with widget state.

There is just one small small problem in the patch:

    // Executing an ajax call is important before saving as it will trigger
    // form state caching and so if for any reasons the form is rebuilt with
    // the entity built based on the user submitted values then the correct
    // mapping will break after the form builder maps over the new form the
    // user submitted values based on the previous delta ordering.

This comment. Can we rewrite it to avoid the part with the problem being the entity being rebuilt with user values? The test doesn't care, we're testing a certain behavior, how and why it works or doesn't work is not relevant.

And of course, the same is true for the issue title and part of the summary. Lets try to describe it simply as what it is, a test for being able to use hidden #value elements in a widget and have it working with re-ordering field items.

And now I need to sleep :)

Fair enough. Title works for me and I can live with that description in the test. I guess it could get out of sync if we'd ever make a change there but I also see the point of describing why it would break with the way that things currently work.

We have everything in the form state to be able to rebuild the form with more items, so altering the entity each time and caching it over and over again is not needed overhead

We build the entity anyway for validation and we also store it back on any request at the moment, so I don't think there would be any additional overhead.

Yes you are right, I did not think about this. But actually this is not a really disruptive entity change, as here only additional (empty) items are added, but nothing is altered and no user input is mapped on top of it.

Not for adding additional items, but it does happen for example in FileWidget, there we replace all existing values. But that has the advantage that having or not having a file results in different form elements, so it doesn't have the reordering problems, I think. Although thinking about it, I think a test that would change alt/title and re-order might have a similar problem as yours.

Ok then, lets do this. Was an important discussion to have and I think we both learned things here. Hopefully that will help us with the other issues.

The overhead will be having to store it over and over again as part of the form cache.

Once the first ajax request happened, we store the form state on every request. That includes the entity. It really makes no performance difference if that is the original entity or it if was changed.

I had a quick look at image fields. The only bug that I could see is that when reordering the items and then uploading a new file, it drops my order change. But if I reorder and save, after uploading new files, it seems to work fine.

I'm genuinely glad that we could reach an agreement here. I was playing around with this locally, and as further justification for the "widgets that do not have all values as user input" use-case I advanced the one from the patch to replace the normal "Add another item" button with "Add rectangle", "Add circle" buttons, etc. That way the "shape" value is never in the user input (like in the patch) and the widget is also actually usable. I actually got it to work somewhat, but the code is pretty shitty, so I'm not actually proposing to put this in the real patch, this is just if someone wants to play around with this as well. I'm also attaching a screenshot to make more clear what I mean. So it's great to have test coverage for this, if someone builds something like this in contrib, we will now never break it!

Really nice to see this RTBC, and great to have an agnostic test for any changes we make in other issue.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!