Media library pager failure (HTTP 400) when editing after previewing a node

robertarias → made their first commit to this issue’s fork.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Proposed solution should be written in the issue summary. Based on the last comment not sure if the solution should be expanded or follow up made.

With a bug a test case should be added to show the issue.

Thanks for the detailed explanation, @robert-arias!

I was able to find what exactly causes the revision_id to go missing when persisting $form_state into private tempstore; that $form_state data is then used to rebuild the entity used for the preview page, as well as to populate the edit form (once the user goes back to editing). The problem is at Drupal\Core\Entity\ContentEntityForm->buildEntity, where the $entity->setNewRevision() call causes the $form_state to not be able to get the revision_id (vid) when serialization happens in NodeForm::preview() (this seems to only occur during the 'submit' pass of FAPI, see call stack below).

Knowing this, I think @robert-arias proposed fix doesn't address the root-cause: the preview submission flow causes the serialized $form_state to miss the revision_id (vid) value, which then causes the preview $entity to be in an incomplete state. Additionally, if the media library widget wants to use the revision_id as part of the anti-tamper hash, we should better try to preserve it.

I'm attaching a patch with a new approach: prevent the $entity->setNewRevision() call for preview submissions, so that the revision_id is preserved when serializing $form_state. I've only done very minimal testing in Drupal 9.5.3, but I'm curious to see what the testbot has to say.

Finally, this is the call stack (Drupal 9.5.3) showing the exact spot where the problematic $entity->setNewRevision() call is performed.

[0] Drupal\Core\Entity\ContentEntityForm->buildEntity @ /path/to/web/core/lib/Drupal/Core/Entity/ContentEntityForm.php:162
[1] Drupal\Core\Entity\EntityForm->submitForm @ /path/to/web/core/lib/Drupal/Core/Entity/EntityForm.php:278
[2] Drupal\Core\Entity\ContentEntityForm->submitForm @ /path/to/web/core/lib/Drupal/Core/Entity/ContentEntityForm.php:145
[3] call_user_func_array:{/path/to/web/core/lib/Drupal/Core/Form/FormSubmitter.php:114} @ /path/to/web/core/lib/Drupal/Core/Form/FormSubmitter.php:114
[4] Drupal\Core\Form\FormSubmitter->executeSubmitHandlers @ /path/to/web/core/lib/Drupal/Core/Form/FormSubmitter.php:114
[5] Drupal\Core\Form\FormSubmitter->doSubmitForm @ /path/to/web/core/lib/Drupal/Core/Form/FormSubmitter.php:52
[6] Drupal\Core\Form\FormBuilder->processForm @ /path/to/web/core/lib/Drupal/Core/Form/FormBuilder.php:595
[7] Drupal\Core\Form\FormBuilder->buildForm @ /path/to/web/core/lib/Drupal/Core/Form/FormBuilder.php:323
[8] Drupal\Core\Controller\FormController->getContentResult @ /path/to/web/core/lib/Drupal/Core/Controller/FormController.php:73
[9] Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult @ /path/to/web/core/modules/layout_builder/src/Controller/LayoutBuilderHtmlEntityFormController.php:39
[10] call_user_func_array:{/path/to/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123} @ /path/to/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123
[11] Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure:/path/to/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:121-124} @ /path/to/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:123
[12] Drupal\Core\Render\Renderer->executeInRenderContext @ /path/to/web/core/lib/Drupal/Core/Render/Renderer.php:580
[13] Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext @ /path/to/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:124
[14] Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure:/path/to/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:96-98} @ /path/to/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php:97
[15] Symfony\Component\HttpKernel\HttpKernel->handleRaw @ /path/to/vendor/symfony/http-kernel/HttpKernel.php:169
[16] Symfony\Component\HttpKernel\HttpKernel->handle @ /path/to/vendor/symfony/http-kernel/HttpKernel.php:81
[17] Drupal\Core\StackMiddleware\Session->handle @ /path/to/web/core/lib/Drupal/Core/StackMiddleware/Session.php:58
[18] Drupal\Core\StackMiddleware\KernelPreHandle->handle @ /path/to/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php:48
[19] Drupal\my_module\StackMiddleware\MyMiddleware->handle @ /path/to/web/modules/custom/my_module/src/StackMiddleware/MyMiddleware.php:123123
[20] Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle @ /path/to/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php:48
[21] Drupal\Core\StackMiddleware\NegotiationMiddleware->handle @ /path/to/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php:51
[22] Stack\StackedHttpKernel->handle @ /path/to/vendor/stack/builder/src/Stack/StackedHttpKernel.php:23
[23] Drupal\Core\DrupalKernel->handle @ /path/to/web/core/lib/Drupal/Core/DrupalKernel.php:713
[24] {main} @ /path/to/web/index.php:19

The failed assert was introduced in #2890758: Block visibility node type not working on preview/revision routes → . I tried finding some evidence there about this assert being intentional (else it may have been written just to match observed behavior). Given I couldn't find any direct evidence for that, let's see if just tweaking the assert to match the new behavior gets all tests to pass.

Linking to question I posted in Slack #contribute, trying to find out for sure if it is intentional for the node-entity-in-preview to not have a revision_id (vid) set: https://drupal.slack.com/archives/C1BMUQ9U6/p1678307331705029

There is a separate bug that can make it difficult to review this one. Adding it to related list.

Thanks for finding that, @jehide! I absolutely overlooked the `::submitForm` called before previewing the node.

I think your solution is the answer to this issue - I knew my fix didn't address the underlying issue, thankfully you found it. I don't think that vid going missing is an intended behavior.

Just updated the MR with #11 (adding a missing docblock + a minor method name change).

Removing 'Needs tests', since just updating an existing test gets the MR to pass.

Removing 'Needs Review Queue Initiative', after updating the issue summary.

Original fix was revert, as per #14. Thanks @robert-arias! :)

Needs Review Queue Initiative doesn't have to do with the issue summary.

Still think additional tests will be needed. Editing the existing tests just to pass I don't think covers the scenario of the bug. But will let the next reviewer decide.

Oops! apologies for the mistake :)

The question about writing a separate test for this is certainly relevant. Of the top of my head, I don't see a different way to test for this than what NodeBlockFunctionalTest.php does; AFAIK, Drupal core does not include a way to render the revision_id for a node entity, in the preview page. Let's see what other reviewers say :).

Seems no one else picked it up.

I tried replicating again for D10.1 following the steps in the issue summary and could not trigger an error.

Is there a step missing in the issue summary?