Pager pagination failure on Media library (HTTP 400) when editing after previewing a node [#3332784]

Problem/Motivation

Trying to use the pager in the media library modal fails (400), if editing an already-saved node, then used preview and then clicked on "Back to content editing".

Steps to reproduce

Install Drupal, use the standard profile
Enable media, media_library
Add a new 'media reference' field under 'article' bundle; allow referencing unlimited media entities with bundle "Image".
Create 25+ media/image entities or visit /admin/structure/views/view/media_library/edit/widget and lower the per-page value to ensure the pager will show.
Create an article node, set a title. Save.
Edit the just-created node.
Click on Preview
Click on "Back to content editing"
Click on "Add Media"
Try to load page 2 of the media library
Notice that a silent failure occurred; checking DevTools/Console, an error is logged about a 400 HTTP response.

The bug reproduces in an existing site running Drupal 9.5.1 with Gin theme.

Proposed resolution

Set the $node->in_preview flag before ContentEntityForm::submitForm() is called.
Update ContentEntityForm::buildEntity(): prevent it from resetting the revision_id (vid) if $node->in_preview is truthy.

This effectively causes the node entity reconstructed from the $form_state serialization to preserve the revision_id (vid).

Remaining tasks

Determine if clearing revision_id (vid) is intentional in the node-in-preview flow.
Review

User interface changes

None

API changes

A $node object build from a serialized $form_state will now have the revision_id (vid) set.

Data model changes

None?

Release notes snippet

Node Preview: a $node object build from a serialized $form_state will now have the revision_id (vid) set.

Comment	File	Size	Author
#11	modal-library-pager-preview-error-3332784-11.patch	3.19 KB	jedihe
#10	modal-library-pager-preview-error-3332784-10.patch	2.12 KB	jedihe
#2	d.o - 3332784-2-repro.mp4	731.4 KB	jedihe

Issue fork drupal-3332784

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3332784-media-library-pager changes, plain diff MR !3499
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

11 January 2023 at 22:52

jedihe created an issue. See original summary.

Status	File	Size
new	d.o - 3332784-2-repro.mp4	731.4 KB

Comment #3

jedihe commented 11 January 2023 at 23:02

Issue summary:

View changes

Comment #4

jedihe commented 11 January 2023 at 23:03

Issue summary:

View changes

Comment #5

jedihe commented 11 January 2023 at 23:12

Bug is also reproducible in Drupal 9.5.1 Umami.

Comment #6

19 February 2023 at 21:00

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

Comment #7

19 February 2023 at 21:35

robertarias opened merge request !3499

Comment #8

robert-arias commented 22 February 2023 at 00:49

Status:

Active

» Needs review

Comment #9

smustgrave commented 22 February 2023 at 19:48

Status:	Needs review	» Needs work
Issue tags:		+Needs Review Queue Initiative, +Needs tests

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.

Comment #10

jedihe commented 8 March 2023 at 17:39

Status	File	Size
new	modal-library-pager-preview-error-3332784-10.patch	2.12 KB

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

Comment #11

jedihe commented 8 March 2023 at 20:44

Status	File	Size
new	modal-library-pager-preview-error-3332784-11.patch	3.19 KB

1 file was hidden/shown/deleted

Status	File	Size
hidden	modal-library-pager-preview-error-3332784-10.patch	2.12 KB

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.

Comment #12

jedihe commented 8 March 2023 at 21:23

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

Comment #13

jedihe commented 9 March 2023 at 14:39

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

Comment #14

robert-arias commented 11 March 2023 at 03:56

Thanks for finding that, @jehide! I absolutely overlooked the `::submitForm` call 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.

Comment #15

jedihe commented 15 March 2023 at 14:09

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

Comment #16

jedihe commented 15 March 2023 at 15:58

Issue summary:	View changes
Status:	Needs work	» Needs review
Issue tags:	-Needs Review Queue Initiative, -Needs tests

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

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

Comment #17

jedihe commented 15 March 2023 at 16:05

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

Comment #18

smustgrave commented 15 March 2023 at 16:07

Issue tags:

+Needs Review Queue Initiative

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.

Comment #19

jedihe commented 16 March 2023 at 20:19

Oops! apologies for the mistake :)

The question about writing a separate test for this is certainly relevant. Off 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 :).

Comment #20

smustgrave commented 6 May 2023 at 23:27

Status:

Needs review

» Needs work

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?

Comment #21

6 May 2023 at 23:27

Version:

10.1.x-dev

» 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.