Problem/Motivation

When a module provides base fields for the User entity type, and uses an entity query in hook_entity_base_field_info() to determine them, the site will get into an infinite loop when clearing caches:
- the entity field manager tries to build the list of field definitions for the User entity type
- the custom hook_entity_base_field_info() implementation will start an entity query
- \Drupal\workspaces\EntityQuery\QueryTrait::prepare() checks whether there's an active workspace
- \Drupal\workspaces\Negotiator\SessionWorkspaceNegotiator::applies() tries to check whether the current user is authenticated
- \Drupal\Core\Session\AccountProxy::getAccount() will try to load the user entity, and eventually ends up trying to build the list of field definitions again

Proposed resolution

Swap the order of the conditions in \Drupal\workspaces\EntityQuery\QueryTrait::prepare().

This fixes the problem because the User entity type can not be supported by workspaces (not revisionable nor publishable), so the isEntityTypeSupported() check will return FALSE and it won't advance to the following hasActiveWorkspace() check.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Release notes snippet

Nope.

CommentFileSizeAuthor
#2 3395626.patch922 bytesamateescu

Issue fork drupal-3395626

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

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
StatusFileSize
new922 bytes

This should do it.

Adding a test for this one-line fix would be quite hard, because we'd have to simulate the conditions described in the issue summary and somehow add a safeguard for the infinite loop. Per #2972776: [policy, no patch] Better scoping for bug fix test coverage, I think we shouldn't require a test here.

amateescu’s picture

Issue summary: View changes
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

As submaintainer of this component going to rely on you that this won't need a test case.

Change seems simple enough and didn't break anything.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Converted the patch to a MR.

catch’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

When a module provides base fields for the User entity type, and uses an entity query in hook_entity_base_field_info() to determine them,

This sounds like a very bad idea, but also something that doesn't need explicit test coverage for. The condition is the same, it's just returning early from the other half.

Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

  • catch committed 4dee9b3a on 10.2.x
    Issue #3395626 by amateescu: Fix workspace-support check in entity...

  • catch committed 2e06d213 on 11.x
    Issue #3395626 by amateescu: Fix workspace-support check in entity...
catch’s picture

Status: Fixed » Closed (fixed)

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