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.

Issue fork drupal-3348390

Command icon 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:

Comments

Grevil created an issue. See original summary.

catch’s picture

Status: Active » Postponed (maintainer needs more info)

This looks like the error is because one of these values is NULL:

     ->condition('vid', $term->bundle())
      ->condition('name', $term->getName())
      ->condition('langcode', $term->language()->getId())

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.

amateescu’s picture

Title: Compatibility issues with "taxonomy_unique" » Improve the way entity forms are "disabled" early when an entity is being edited in a workspace
Category: Bug report » Task
Status: Postponed (maintainer needs more info) » Active

The 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.

grevil’s picture

@amateescu, great find!

Maybe someone with more insights could have a look!

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.

jklmnop’s picture

I'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.

Deprecated: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in /var/www/html/web/core/lib/Drupal/Core/Url.php on line 281

Deprecated: parse_url(): Passing null to parameter #1 ($url) of type string is deprecated in /var/www/html/web/core/lib/Drupal/Core/Url.php on line 284

Deprecated: strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in /var/www/html/web/core/lib/Drupal/Core/Url.php on line 289
s_leu’s picture

Status: Active » Needs review
StatusFileSize
new3.33 KB

I 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:

  • Install the workspaces module
  • Add a link field to the page content type, cardinality unlimited, standards for other settings
  • Create a content of type page, adding a link with values and clicking the "Add another item" button not populating the 2nd link item
  • Save the node and re-open the edit form

The mentioned notices will be displayed. Here one with a full backtrace:

Deprecated function: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in Drupal\Core\Url::fromUri() (line 281 of core/lib/Drupal/Core/Url.php).
Drupal\Core\Url::fromUri(NULL, Array) (Line: 175)
Drupal\link\Plugin\Field\FieldType\LinkItem->getUrl() (Line: 27)
Drupal\link\Plugin\Validation\Constraint\LinkTypeConstraintValidator->validate(Object, Object) (Line: 202)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints(Object, '000000000000058c0000000000000000', Array) (Line: 154)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object) (Line: 164)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object) (Line: 164)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode(Object, Array, 1) (Line: 106)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate(Object, NULL, NULL) (Line: 93)
Drupal\Core\TypedData\Validation\RecursiveValidator->validate(Object) (Line: 132)
Drupal\Core\TypedData\TypedData->validate() (Line: 485)
Drupal\Core\Entity\ContentEntityBase->validate() (Line: 279)
Drupal\workspaces\EntityOperations->entityFormAlter(Array, Object, 'node_page_edit_form') (Line: 64)
workspaces_form_alter(Array, Object, 'node_page_edit_form') (Line: 545)
Drupal\Core\Extension\ModuleHandler->alter('form', Array, Object, 'node_page_edit_form') (Line: 840)
Drupal\Core\Form\FormBuilder->prepareForm('node_page_edit_form', Array, Object) (Line: 284)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 592)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 166)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 74)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

The problem is basically that the following code in \Drupal\workspaces\EntityOperations::entityFormAlter validates all entity constraints every time it runs on a form of a workspace supported entity type:

    foreach ($entity->validate()->getEntityViolations() as $violation) {
      if ($violation->getConstraint() instanceof EntityWorkspaceConflictConstraint) {
        $form['#markup'] = $violation->getMessage();
        $form['#access'] = FALSE;
        continue;
      }
    }

Instead we could just only validate the acutally workspace related constraints, adding a patch which will do exactly that.

smustgrave’s picture

Even though this is a task seems like the change that should have test coverage?

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Ran 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!

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

I'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.

amateescu’s picture

Status: Needs work » Needs review

Started 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 :)

amateescu’s picture

Issue summary: View changes

Updated the issue summary.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Believe 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.

amateescu’s picture

Category: Task » Bug report

This 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.

  • catch committed 92cbe86b on 10.3.x
    Issue #3348390 by amateescu, s_leu, Grevil: Improve the way entity forms...

  • catch committed a91aedcf on 10.4.x
    Issue #3348390 by amateescu, s_leu, Grevil: Improve the way entity forms...

  • catch committed 1754440b on 11.x
    Issue #3348390 by amateescu, s_leu, Grevil: Improve the way entity forms...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Had 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!

  • catch committed 1754440b on 11.0.x
    Issue #3348390 by amateescu, s_leu, Grevil: Improve the way entity forms...

Status: Fixed » Closed (fixed)

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