Problem/Motivation

#2570593: Allow entities to be subclassed using "bundle classes" introduced a BC layer in core/lib/Drupal/Core/Entity/EntityStorageBase.php for legacy code that directly accesses the now removed entityClass protected property.

We can remove all this complication once 10.0.x branch is open.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

larowlan’s picture

Title: [PP-1] Remove BC layers in EntityStorageBase for direct access to entityClass property » Remove BC layers in EntityStorageBase for direct access to entityClass property
Status: Postponed » Active
Spokje’s picture

Spokje’s picture

Status: Active » Needs review
longwave’s picture

Status: Needs review » Reviewed & tested by the community

Only removals, no additions - looks good to me.

dww’s picture

Status: Reviewed & tested by the community » Needs review

Thanks for working on this!

  1. I wrote a patch for this when this code was first landing, and I think it did a bit more cleanup of comments and such. Need to double check that (but I’m on my phone right now).
  2. I thought we weren’t yet removing deprecations and we’re trying to get a D10 release only with dependency updates (mostly Symfony) first, tag an alpha, then worry about deprecation removals. Maybe this shouldn’t be sitting in the RTBC queue for months, triggering testbot reruns every 2 days, etc?

Back to NR for at least 1, perhaps 2 as well.

dww’s picture

catch’s picture

Title: Remove BC layers in EntityStorageBase for direct access to entityClass property » [PP-1] Remove BC layers in EntityStorageBase for direct access to entityClass property
Status: Needs review » Postponed

Yes IMO we should make 10.0.0-alpha1 as much as possible 9.4 + dependency updates, so that contrib can get a deprecation report for as much as possible without having to update anything yet. Then once it's out start committing deprecation removals.

Going to mark as postponed on the alpha1 issue for now.

Spokje’s picture

Thanks @dww and @catch, I got carried away in my sheer enthusiasm to get rid of deprecations.

Will stop working on those until 10.0.0-alpha1 is out.

fgm’s picture

Returning $this->entityType->getClass() may cause a problem because it can return a NULL but the signature does not allow it. This causes issues like #3259941: getEntityClass() must be of the type string, null returned.

--- copied from closed issue #3246150: Bundle class changes mean the entity class is now determined by the active entity-type definition -----

This change introduces appears to be causing a compatibility issue described at #3259941: getEntityClass() must be of the type string, null returned where the EntityStorageBase::baseEntityClass is not set, and returns a badly typed result:

 public function getEntityClass(?string $bundle = NULL): string {
    // @todo Simplify this in Drupal 10 to return $this->entityType->getClass().
    // @see https://www.drupal.org/project/drupal/issues/3244802
    return $this->baseEntityClass ?? $this->entityType->getClass();
  }

That function returns a NULL when the baseEntityClass is not set and $this->entityType->getClass() returns NULL too (the signature of that function allows an empty return), but the function signature does not allow for NULL results.

Rewriting it like this fixes the problem, but it may well be a symptom of a deeper problem

    return ($this->baseEntityClass ?? $this->entityType->getClass()) ?? "";
Spokje’s picture

Title: [PP-1] Remove BC layers in EntityStorageBase for direct access to entityClass property » Remove BC layers in EntityStorageBase for direct access to entityClass property
Status: Postponed » Needs work

Unpostponing

andypost’s picture

Status: Needs work » Needs review
FileSize
9.71 KB
22.57 KB

Removed remains from the Entity namespace

Status: Needs review » Needs work

The last submitted patch, 13: 3244802-13.patch, failed testing. View results

andypost’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
23.68 KB

remove legacy test

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/Query/Sql/Query.php
@@ -137,8 +137,7 @@ protected function prepare() {
     if (is_null($this->accessCheck)) {
-      $this->accessCheck = TRUE;
-      @trigger_error('Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or FALSE to specify whether access should be checked. See https://www.drupal.org/node/3201242', E_USER_DEPRECATED);
+      throw new QueryException('Entity query must specify whether access should be checked.');

+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php
@@ -1358,11 +1359,10 @@ public function testToString() {
-   * @group legacy
    */
   public function testAccessCheckSpecified() {
-    $this->expectDeprecation('Relying on entity queries to check access by default is deprecated in drupal:9.2.0 and an error will be thrown from drupal:10.0.0. Call \Drupal\Core\Entity\Query\QueryInterface::accessCheck() with TRUE or FALSE to specify whether access should be checked. See https://www.drupal.org/node/3201242');
+    $this->expectException(QueryException::class);
+    $this->expectExceptionMessage('Entity query must specify whether access should be checked.');
     $this->storage->getQuery()->execute();

probably could use better wording

daffie’s picture

Status: Needs review » Reviewed & tested by the community

I shall leave it up to the committer to decide if the query exception message should be improved and if so to what exactly. For me it is good enough.
All code changes look good to me.
For me it is RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Opened some phpstan-drupal issues for the deprecation removals.

https://github.com/mglaman/phpstan-drupal/issues/336
https://github.com/mglaman/phpstan-drupal/issues/335
https://github.com/mglaman/phpstan-drupal/issues/334

On the exception message:

Entity query must specify whether access should be checked.

Maybe this?
Entity queries must explicitly set whether the query should be access checked or not. See Drupal\Core\Entity\Query\QueryInterface::accessCheck()

catch’s picture

Title: Remove BC layers in EntityStorageBase for direct access to entityClass property » Remove BC layers in entity system

Widening the title to match the patch a bit better.

andypost’s picture

FileSize
1.51 KB
23.84 KB

Fixed the exception message

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks better in the patch :) Ready to go once the bot is green.

  • catch committed d47cd98 on 10.0.x
    Issue #3244802 by andypost, Spokje, catch, dww: Remove BC layers in...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed d47cd98 and pushed to 10.0.x. Thanks!

Status: Fixed » Closed (fixed)

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