Problem/Motivation
When adding a new entity, \Drupal\workspaces\EntityOperations::entityFormAlter() runs all entity validation constraints when the entity form is being rebuilt via AJAX, before the form values are extracted and set on the entity object.
This is problematic because some constraints don't take into account the case of new entities, which might not be fully populated at that time.
Steps to reproduce
- Install both "workspaces" and "taxonomy unique"
- Add a taxonomy vocabulary and enable the "Terms should be unique." option
- Add a term.
- The error gets thrown.
Proposed resolution
Validate only the EntityWorkspaceConflictConstraint constraint, and move the code to workspaces.controller.entity_form to be in line with the solution that was developed for #3092247: Prevent content from being deleted when there is an active workspace.
Remaining tasks
Review.
User interface changes
Nope.
API changes
Nope.
Data model changes
Nope.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3348390
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3348390-improve-the-way-entity-forms-are-disabled
changes, plain diff MR !7740
Comments
Comment #2
catchThis looks like the error is because one of these values is NULL:
Can you add debug where the query is built, to check whether this is the case for any of them? If this is the case, and you can confirm that it doesn't happen when workspaces isn't enabled, that might be a way to track this down.
Comment #3
amateescu commentedThe problem here is
Drupal\workspaces\EntityOperations::entityFormAlter(), which runs entity validation before the term object is fully populated with data. I posted a patch in the contrib issue, but let's use this one to explore other approaches for "disabling" entity forms when a certain entity is being edited inside a workspace.Comment #4
grevil commented@amateescu, great find!
Maybe someone with more insights could have a look!
Comment #6
jklmnop commentedI'm also having this problem, but with Drupal 9.5.10 on PHP 8.1 and Link fields.
I'm using a Link field with unlimited values, but this also happens when you have limited values >1 but not all of them are being used. This is due to this pre-validation on the form load that will validate the empty placeholder fields (which have a null value).
I've tried it on a fresh install (versions are above) with only Workspaces turned on, created a content type with an unlimited Link field. Created a node with at least one value in the Link field. Went to the edit form for the node to see the deprecation notices. This would likely be fatal in PHP 8.2.
Comment #7
s_leu commentedI can confirm the notices posted in #6, same scenario, a link field with unlimited values. Here the full steps to replicate on Vanilla Drupal 10.1.x:
The mentioned notices will be displayed. Here one with a full backtrace:
The problem is basically that the following code in
\Drupal\workspaces\EntityOperations::entityFormAltervalidates all entity constraints every time it runs on a form of a workspace supported entity type:Instead we could just only validate the acutally workspace related constraints, adding a patch which will do exactly that.
Comment #8
smustgrave commentedEven though this is a task seems like the change that should have test coverage?
Comment #9
smustgrave commentedRan the 11.x tests on #7 and all green.
Brought up in #workspace slack channel and @amateescu brought up this already had test coverage at \Drupal\Tests\workspaces\Functional\WorkspaceConcurrentEditingTest::testConcurrentEditing
LGTM!
Comment #10
quietone commentedI'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions.
I read the patch, which should be testing against 11.x, and see that it is adding a constructor parameter. Because of that we need to follow the deprecation policy for Constructor parameter additions.
There is also no test here. However, there is a brief explanation in #9.
I am setting this back to Needs Work for the deprecation notice.
Comment #12
amateescu commentedStarted a MR with a new approach that builds upon the entity form controller override that was added in #3092247: Prevent content from being deleted when there is an active workspace. That service is new in 10.3.x, so it doesn't need any deprecation handling :)
Comment #13
amateescu commentedUpdated the issue summary.
Comment #14
smustgrave commentedBelieve probably good to get this in before 10.3 so don't need a CR for the new service parameter.
Leaning on tests being green, applied the MR locally and tested basic functionality of workspace
Create a space
Create content in the stage space
Publish content
Believe this refactor is good.
Comment #15
amateescu commentedThis is actually a bug because in HEAD, if you have Layout Builder enabled and edit an entity in a workspace, when you visit `/node/X/layout` in Live, the concurrent editing message is not displayed. Added a test for this problem and keeping the issue at RTBC because it's a simple test addition with no other functional change to the MR.
Comment #19
catchHad two minor points on the MR, one of which was wrong because this is new code in 10.3 anyway, and the other was a small change that's done.
Committed/pushed to 11.x and cherry-picked to 10.4.x and 10.3.x, thanks!