Only one additional new value saved unlimited field and no non-field values are restored after preview

here's a failing test

The issue here is $preview = $form_state loses the $form_state->getStorage('field_storage') when the NodeForm::form method returns.

No idea why

So $form_state = $preview is invalid as it doesn't persist outside NodeForm::form (not passed by reference).

So we need to copy the values over.

This fixes the issue *but* breaks the reload feature (seen as test fails)
But I don't think we can have both, as allowing reload means we keep loading the stale form state from the tempstore using the UUID.

I think the fix is to remove those three asserts that check reloading retains the data.

@swentel - is there a reason we support reloading for the unsaved node (but not for saved nodes)?

Hmm, the only reason I can think of (right now) is that if you reload node/add/{x}, you'd lose all entered data. However, that's only the case if you have gone to the preview at least once since we have an entry in the tempstore at that point - or at least in current HEAD.

I'm fine with removing that 'feature'. Reloading is a deliberate action by either clicking on the reload button in your browser or using a keyboard shortcut. Also, in case you'd click away and hit the back button, you're also still fine.

Ok removes tests for that feature

Code looks fine to me - added manual testing just for a double confirmation, didn't get to that yet.

+++ b/core/modules/node/src/Tests/PagePreviewTest.php
@@ -149,13 +178,7 @@ function testPagePreview() {
+    ¶

Extreme nitpick - spaces :)

Removed the newline

Did some manual testing on the node article which I made unlimited.
It has some weird side effects like losing the alternate text, or even a second image when coming back to the node form - unless that is already in HEAD, ran out of time to test that.

Confirming that alt text getting lost after previewing is in head.
But if you add the alt text again, preview for a 2nd time, it is still there when going back.

Quick chat with @larowlan - going to send that to a new issue.

Added #2551217: Data loss using Preview on content types with image field.

Lee asked me about this in IRC. I had no idea that you could hit reload in your browser and get your old form contents back. I would never in my life expect that to happen, since it doesn't happen literally anywhere else on the entire internet. :) Reload = destructive. (Honestly, by design. It's the "oh crap, abort abort!" button.) So I'm fine to kill that feature, as long as we don't screw Views UI in the process, in case people were counting on that for that UI. (I don't see any hunks outside of Node so I think we're fine there.)

Tested it again - looks good now. #2551217: Data loss using Preview on content types with image field. fixes the actual data loss then.

By mistake I had removed the code changes that are already committed. Still the tests will fail (tested on local). Will look into them later.

looking

So the issue here is that FileWidget::submit stores the uploaded items in storage.

Which takes precedence over $items taken from FormStateInterface::getValues() in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::formMultipleElements

    // Load the items for form rebuilds from the field state as they might not
    // be in $form_state->getValues() because of validation limitations. Also,
    // they are only passed in as $items when editing existing entities.
    $field_state = static::getWidgetState($parents, $field_name, $form_state);
    if (isset($field_state['items'])) {
      $items->setValue($field_state['items']);
    }

and in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::submit

    // Update items.
    $field_state = static::getWidgetState($parents, $field_name, $form_state);
    $field_state['items'] = $submitted_values;
    static::setWidgetState($parents, $field_name, $form_state, $field_state);

So let's see what if this breaks anything.

Ok, instead of clearing it, we should update it.

In my books this is ready now

Will look and do manual test today

So there's still a problem here - unless we have a different issue for that ?

1) Set image to unlimited
2) Add an image
3) Click preview
4) Go back to node edit
5) Add another image
6) it won't work
7) Try to add again, now it will work
8) Click preview and you'll see the second image
9) Go back to node edit
10) Hit preview again, second image will be gone.

Tricky one :/

Will try to check later today to write a test for #32

Patch that mimics the scenario from #32 until step 6 proving the fail to upload the second image after coming back from preview.

Extended tests. Added a new approach, it seems the node is not saved with the change. Let's see with testbot.

+++ b/core/modules/node/src/NodeForm.php
@@ -89,7 +94,12 @@ public function form(array $form, FormStateInterface $form_state) {
+          // We need to store the uuid in a new key to take it from there on
+          // preview.
+          $store->set('post_' . $uuid, $preview);

not on preview, on form submission.

diff --git a/core/modules/node/src/NodeForm.php b/core/modules/node/src/NodeForm.php
index 6165d74..6d13454 100644
--- a/core/modules/node/src/NodeForm.php
+++ b/core/modules/node/src/NodeForm.php
@@ -76,7 +76,7 @@ public function form(array $form, FormStateInterface $form_state) {
     }
 
     $key = $uuid;
-    if (\Drupal::request()->isMethod('POST')) {
+    if (\Drupal::request()->isMethod('POST') && !$this->entity->isNew()) {
       $key = 'post_' . $uuid;
     }
 

Add that, then adding content works again.

True, test passes locally with it, let's see about test bot. :)

Turns out my idea with post_$uuid doesn't work, it would still be used if you refresh the page and then submit.

Discussed with @swentel and I have a different idea:

(12:48:08) berdir: swentel: the post_$uuid trick is passing both the existing and our new tests in https://www.drupal.org/node/2548713#comment-11085423. However, it occured to me that would also use that key if you refresh the page and then submit :-/
(12:50:25) swentel: berdir, hmm, and can we remove the refresh 'feature' ? Because that's fine for me, also for webchick.
(12:50:40) berdir: swentel: what if we also use the ?uuid= argument for edit?
(12:50:48) berdir: swentel: and only load from state there, but if present, always do it
(12:50:56) berdir: and support refreshing, which then kind of makes sense..
(12:51:28) swentel: berdir, works for me, add is using that too

So.... in the back to content link, always append the uuid argument. And in the code, only do it if that argument is present. And then remove the delete completely, then a refresh will keep the data, but given that we have that special argument, that makes sense to me.

Thank you for the feedback! :D
Seems to be working, let's see about testbot.

Discussed with @Berdir this will help with consistency.

Love the cleanup, makes the code more consistent in general.

+++ b/core/modules/node/src/NodeForm.php
@@ -73,9 +73,8 @@ public function form(array $form, FormStateInterface $form_state) {
+      $form_state->setStorage($preview->getStorage());

We'll need a test for this line. Talked with berdir in IRC, and probably a simple form alter on the node form which puts something in the storage should do the trick for testing this.

Added code of a previous patch. Could not manage to fix the tests already written.

Ok, the problem with the previous patch was that we always took the storage now and replaced it if it was updated already. Now we skip that if we are rebuilding and have our own storage already.

Also, as far as I can see, setting the form values is not necessary, the test passes without that and manual testing is fine too.

We still need test coverage that form storage works in the first place.

Thank you so much @Berdir for helping with the patch! Added test coverage. If it passes should be Ok now. :)

Sorry, here we go with the test-only patch.

Test fail was just the test-only patch.

1. +++ b/core/modules/node/src/NodeForm.php
   @@ -67,31 +67,21 @@ protected function prepareEntity() {
   +    $request_uuid = \Drupal::request()->query->get('uuid');
   

   Nit: We can inject the request stack although I note that this is existing in HEAD

2. +++ b/core/modules/node/src/NodeForm.php
   @@ -67,31 +67,21 @@ protected function prepareEntity() {
   -        $store->delete($uuid);
   

   For other reviewers note that this is cleaned up when the form is submitted and the node is saved (see \Drupal\node\NodeForm::save)

+++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
@@ -288,6 +288,14 @@ public function massageFormValues(array $values, array $form, FormStateInterface
+    // Update reference to 'items' stored during upload to take into account
+    // changes to values like 'alt' etc.
+    // @see \Drupal\file\Plugin\Field\FieldWidget\FileWidget::submit()
+    $field_name = $this->fieldDefinition->getName();
+    $field_state = static::getWidgetState($form['#parents'], $field_name, $form_state);
+    $field_state['items'] = $new_values;
+    static::setWidgetState($form['#parents'], $field_name, $form_state, $field_state);
+

Is it possible some contrib widgets will need similar fixes?

Setting back to needs review to get an answer to #58

Yes, I guess it is possible.

But I don't see an alternative. We have to restore form storage. Other things are broken right now because that's missing, we're just not seeing it.

To be honest, I don't really understand what the code in FileWidget is even doing. Why is it the only widget affected by this. How is it not a problem for all other widgets? Why is that even needed?

The only other two modules that I have locally that use those widget state methods are Paragraphs and Inline Entity Form. I've just run all paragraphs tests and they are passing with this patch. We do have preview tests, quite a few actually. In fact, we even have a commented out assertion that is blocked on this issue because things are pretty broken without this fix.

And IEF doesn't support previews at all right now.

The core committers and Entity and Field maintainers agreed that this issue was a major bug. See #60 for some of the impacts of this; the issue is apparently quite disruptive when combined with a workflow that involves a lot of previewing (as is common with paragraph).

berdir pinged me to look at this issue. The patch looks sane, I agree with the general approach.

It makes sense for a widget that uses widget state to keep that state up to date with changes to $items, but I'm unsure why the logic in FileWidget::submit() isn't enough (perhaps submit() doesn't trigger when needed?)

I've also had a quick look on the widget state code that this patch adds. As @bojanz said, the same routine happens in ::submit, but it isn't triggered, but I have no clue why yet either. Makes me wonder whether we can remove that code from the submit method if this now would live in massageFormValues() too ?

Re #58.

I think it is pretty unlikely that another widget needs this. File upload is very special, it is the only one in core that uses widget state for this, everything else just works. It has to do with the very strange #type file_managed and how it is processing values.

The reason we do need it now is that there can be case where a file is uploaded and ::submit() is not called, for example when having JS disabled and directly clicking on preview.

IMHO, even if we would break some widget for preview, this is fixing worse bugs in core that affect pretty much all field types than breaking a widget. So setting back to RTBC, lets discuss if you're not convinced.

Manually testing HEAD (8.3.x) with the steps in the current IS, I'm not experiencing the bug described.

Looking at the test in the patch, I'm not seeing those steps either. Instead I'm seeing this:

+++ b/core/modules/node/src/Tests/PagePreviewTest.php
@@ -260,6 +284,52 @@ function testPagePreview() {
+    $this->drupalPostForm(NULL, $edit, t('Preview'));
+    $this->clickLink(t('Back to content editing'));
+    $this->drupalPostForm(NULL, $edit, t('Preview'));
+    $this->clickLink(t('Back to content editing'));

Which is testing what happens when you go to preview and back multiple times. Which is a great test, and is uncovering a real bug in HEAD, but it's different than this issue's title and summary.

I'm not sure if this needs to be set to "Needs work" and "Needs issue summary update", or if the issue title and summary are correct, and there's something I'm doing wrong in testing this. Therefore, setting to "Needs review" for feedback on that.

Sorry about the old isse summary. I updated it and also slightly changed the title to better explain the problem.

This breaks values from non-fields, e.g. menu_ui. We apparently have no test coverage for that, so we need to add that as well.

1. +++ b/core/modules/node/src/NodeForm.php
   @@ -67,31 +67,21 @@ protected function prepareEntity() {
   -      foreach ($preview->getValues() as $name => $value) {
   -        $form_state->setValue($name, $value);
   -      }
   

   This is likely the reason for #69.

2. +++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
   @@ -288,6 +288,14 @@ public function massageFormValues(array $values, array $form, FormStateInterface
   +    // Update reference to 'items' stored during upload to take into account
   +    // changes to values like 'alt' etc.
   +    // @see \Drupal\file\Plugin\Field\FieldWidget\FileWidget::submit()
   +    $field_name = $this->fieldDefinition->getName();
   +    $field_state = static::getWidgetState($form['#parents'], $field_name, $form_state);
   +    $field_state['items'] = $new_values;
   +    static::setWidgetState($form['#parents'], $field_name, $form_state, $field_state);
   

   massageFormValues() shouldn't have a side-effect like this. I think this makes more sense in extractFormValues(), where WidgetBase already does similar state manipulation. I wonder if we should split this whole bit out into a separate issue? Is it possible to trigger the problem this is solving independently of what the rest of this patch is doing? Seems to me like it should be, but I haven't delved into trying.

   Related: it bothers me that with this patch, we would have this code duplicated between FileWidget::extractFormValues() and FileWidget::submit(). In fact, looks like FileWidget::submit() already duplicates some of WidgetBase::extractFormValues(). I wonder if we could/should somehow make FileWidget::submit() invoke extractFormValues() rather than duplicate it? Possibly tricky due to the former being a static function, and maybe a bad idea for other reasons, just throwing it out there as a first impression.

Also, thank you for the updated IS, but:

- press Save
- Only the first additional value (in total 2) was saved.

The test in the patch is not testing this. It's testing something similar, via multiple successive previews, but I think it's worth having an explicit test for the steps in the issue summary, since that appears a more direct pathway to data loss.

Yeah, it is a bit more complicated than just restoring that code, because that actually don't really do anything. We restore form values from user input, not values. And restoring that makes it want to cache, so I have to trick it into believing it is a post request. Not sure if that has side effects.

One side effect of this is that the exact submitted values are restored and we don't rebuild based on the built entity. That means it requires a small change in the existing test coverage, because the term field is exactly as it was entered, without the (1) part.

I'm also sure that this is *not* a regression from HEAD, as setting form state values doesn't do anything.

I also added test coverage for actually saving and ensuring the value is there. And I moved the widget state stuff.

Still looking for reviews here :)

Also, I'm still surprised myself, but I actually *understand* FileWidget now I think. See #2811841-32: Add test coverage ensuring user input is mapped on the correct form elements when elements are reordered and the issue in general for a lot of fun facts about entity forms, widgets, widget state and more.

+++ b/core/modules/file/src/Plugin/Field/FieldWidget/FileWidget.php
@@ -288,15 +288,19 @@ public function massageFormValues(array $values, array $form, FormStateInterface
+
+  public function extractFormValues(FieldItemListInterface $items, array $form, FormStateInterface $form_state) {

Nit: {@inheritdoc}

I didn't know about getUserInput() this far, so would be nice if someone else could confirm that this is ok (although tests prove if of course).

1. +++ b/core/modules/node/src/NodeForm.php
   @@ -67,31 +67,28 @@ protected function prepareEntity() {
   -      foreach ($preview->getValues() as $name => $value) {
   -        $form_state->setValue($name, $value);
   -      }
   ...
   +      $form_state->setUserInput($preview->getUserInput());
   ...
          $form_state->setRebuild();
   ...
   +      $form_state->setRequestMethod('POST');
   

   This will be the first usage of setUserInput() outside of Views code :)
   It also happens to be correct usage here.

2. +++ b/core/modules/node/src/NodeForm.php
   @@ -67,31 +67,28 @@ protected function prepareEntity() {
   +      $form_state->setStorage($preview->getStorage());
   

   Looking at \Drupal\Core\Form\FormState::getCacheableArray(), pretty sure that storage is the only part we care about, so this looks correct.

Fixed the nitpick. This should be ready then?

Yes!

I've manually tested this and it fixes the bug. There are no changes that contradict our BC policy.

Committed and pushed f403ccb to 8.3.x and 5d0814d to 8.2.x. Thanks!

diff --git a/core/modules/node/tests/modules/node_test/node_test.module b/core/modules/node/tests/modules/node_test/node_test.module
index 55149f4..866be19 100644
--- a/core/modules/node/tests/modules/node_test/node_test.module
+++ b/core/modules/node/tests/modules/node_test/node_test.module
@@ -10,9 +10,11 @@
 
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\Display\EntityViewDisplayInterface;
+use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\node\NodeInterface;
 
+
 /**
  * Implements hook_ENTITY_TYPE_view() for node entities.
  */
@@ -179,7 +181,7 @@ function node_test_node_insert(NodeInterface $node) {
 /**
  * Implements hook_form_alter().
  */
-function node_test_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
+function node_test_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   if (!$form_state->get('node_test_form_alter')) {
     drupal_set_message('Storage is not set');
     $form_state->set('node_test_form_alter', TRUE);

Fixed on commit.

Argh, I did find one regression here, actually identifed by a paragraph test that I didn't run when working on this (ParagraphsAccessTest) but took me a while to track it down.

Given the following:

* An Image field (probably file as well), does not need to be on paragraph/nested form, the one on article works to reproduce this
* JS disabled
* alt/title not required

When you select a file, and then directly go to Preview, without clicking "Upload" first, and then go back to content edit then the image field is in a weird, messed up state. Somehow I think we get a mess between user input and stored widget state so that the fids input overrides the actual data.

Saving works fine without clicking upload, it's really just Preview.

I *think* this is rare enough to not require that this is reverted but can be looked into in a follow-up, thoughts?

I'm fine with follow up

Ok, created #2836455: Image/file lost after node preview without JS