Problem/Motivation

Discovered by @jeqq while testing the core Workspace module.

Steps to reproduce:

- enable the workspace module
- switch to a non-default workspace
- add an article node and try to upload an image (or remove the image from an existing article)

The problem lies within \Drupal\workspaces\EntityOperations and \Drupal\workspaces\FormOperations which are adding an entity builder and a form validate callback to all entity forms, without taking care of managing their own injected dependencies.

Proposed resolution

One solution would be to use DependencySerializationTrait in both those objects, but that's just a workaround and the usage of that trait should not be actively encouraged.

Instead, they should add their form callbacks as static methods, which ensures that the objects themselves are not being serialized when the form is being serialized in the db cache.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

CommentFileSizeAuthor
#8 2998565-8.patch5.15 KBamateescu
#8 2998565-8-test-only.patch2.07 KBamateescu
#8 poc-interdiff.txt1.17 KBamateescu
#7 poc.txt2.1 KBsam152
#2 2998565.patch6.68 KBamateescu
#2 2998565-test-only.patch3.6 KBamateescu

Comments

amateescu created an issue. See original summary.

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2998565-test-only.patch3.6 KB
new 2998565.patch6.68 KB

This should do it.

amateescu credited jeqq.

amateescu’s picture

amateescu commented

The last submitted patch, 2: 2998565-test-only.patch, failed testing. View results

sam152’s picture

sam152 commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/workspaces/tests/src/FunctionalJavascript/WorkspaceFunctionalIntegrationTest.php
@@ -0,0 +1,99 @@
+/**
+ * Tests various UI interaction in entity forms with the Workspaces module.
+ *
+ * @group workspaces
+ */
+class WorkspaceFunctionalIntegrationTest extends WebDriverTestBase {

I wonder if we could test these kinds of issues in the future by building the form and attempting to serialize/unserialize it in a kernel test. JS tests are a bit of a headache.

Patch looks good to me.

sam152’s picture

sam152 commented
StatusFileSize
new poc.txt2.1 KB

Here is a POC for #6. I'll leave it out of a patch and keep RTBC for #2 if a full blown functional test is preferred here.

amateescu’s picture

amateescu commented
StatusFileSize
new poc-interdiff.txt1.17 KB
new 2998565-8-test-only.patch2.07 KB
new 2998565-8.patch5.15 KB

Whoa, I love the much simpler way of testing this bug from #7!

+++ b/core/modules/workspaces/tests/src/Kernel/WorkspaceIntegrationTest.php
@@ -718,4 +720,35 @@ protected function flattenExpectedValues(array $expected, $entity_type_id) {
+   * Test to ensure non-default workspace entity forms are cacheable.
...
+   * Test to ensure forms with non-default workspace validation are cacheable.

The term "cacheable" has a very loaded meaning in D8, usually referring to the render cache. So I changed these descriptions a bit to mention that we're testing the form cache :)

The last submitted patch, 8: 2998565-8-test-only.patch, failed testing. View results

sam152’s picture

sam152 commented

Changes look good to me. +1 RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2998565-8.patch, failed testing. View results

sam152’s picture

sam152 commented
Status: Needs work » Reviewed & tested by the community

Infra hiccup.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Nice test :)

Committed and pushed 9ff7e11bd7 to 8.7.x and 77584d1c3a to 8.6.x. Thanks!

  • alexpott committed 9ff7e11 on 8.7.x 
    Issue #2998565 by amateescu, Sam152, jeqq: Workspaces' form alters break...

  • alexpott committed 77584d1 on 8.6.x 
    Issue #2998565 by amateescu, Sam152, jeqq: Workspaces' form alters break...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.