Problem/Motivation

We should remove usages of entity.query service from core and properly deprecate in line with policy as the Symfony container has already worked out that the service is deprecated.

Proposed resolution

Remove all usages in favour of using the storage or the specific services.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#14 2968519-14.patch86.29 KBalexpott
#9 2968519-9.patch86.36 KBalexpott
#9 5-9-interdiff.txt3.57 KBalexpott
#5 2968519-5.patch83.59 KBalexpott
#5 2-5-interdiff.txt31.14 KBalexpott
#2 2968519-2.patch71.16 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 2968519-2.patch71.16 KB
amateescu’s picture

amateescu commented 
  1. +++ b/core/modules/options/options.module
@@ -122,8 +122,7 @@ function options_field_storage_config_update_forbid(FieldStorageConfigInterface
+    $result = \Drupal::entityQuery($entity_type)

    Why don't we go through the storage here and in all the other places which are changed to use \Drupal::entityQuery()? 

  2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityQueryAggregateTest.php
@@ -23,7 +23,7 @@ class EntityQueryAggregateTest extends EntityKernelTestBase {
-   * @var \Drupal\entity_test\EntityTestStorage
+   * @var \Drupal\Core\Entity\EntityStorageInterface

    Is there any reason for this change?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@amateescu thanks for the reivew.

  1. Less code - it goes through the storage. It's procedural so using the shortest and less code route through \Drupal seemed best - especially given ::entityQuery() exists.
      public static function entityQuery($entity_type, $conjunction = 'AND') {
    return static::entityTypeManager()->getStorage($entity_type)->getQuery($conjunction);
  }
  2. It doesn't exist as far as I could see.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 2-5-interdiff.txt31.14 KB
new 2968519-5.patch83.59 KB

But I agree that we shouldn't use \Drupal::entityQuery in the EntityQueryTest kernel test - just feels wrong - so was using the entity query service but no reason not to fix it here considering we're changing it.

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Right, the entity test storage class doesn't exist anymore. In that case, the patch looks good to me :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/core.services.yml
@@ -879,6 +879,7 @@ services:
+    deprecated: The "%service_id%" service is deprecated. Use \Drupal\Core\Entity\EntityStorageInterface::getQuery() or \Drupal\Core\Entity\EntityStorageInterface::getAggregateQuery() instead.

I think this should tell you to get the entity storage from the entity type manager service or similar explicitly, there's not really much to go on to figure out what to do instead in the message.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I copied the message from \Drupal\Core\Entity\Query\QueryFactory

 * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0. Use
 *   \Drupal\Core\Entity\EntityStorageInterface::getQuery() or
 *   \Drupal\Core\Entity\EntityStorageInterface::getAggregateQuery() instead.

I'll improve both and link to the CR.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 5-9-interdiff.txt3.57 KB
new 2968519-9.patch86.36 KB

Updated the service deprecation and properly deprecated EntityQuery plus removed a couple of incorrect references from test docs.

martin107’s picture

martin107 commented

There is a large amount of intricate changes here....

For what it is worth - I have taken the time to scan the patch slowly....
talking time to justify that all changes are equivalent, without any subtle buggy differences.

At the end, I could say every change was justifiable

So +1 from me.

Oh yeah and the idea behind the issue is good :)

amateescu’s picture

amateescu commented
Status: Needs review » Reviewed & tested by the community

Looks good to me as well :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2968519-9.patch, failed testing. View results

martin107’s picture

martin107 commented
Issue tags: +Needs reroll

Just sending up the bat signal

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
StatusFileSize
new 2968519-14.patch86.29 KB

Rerolled.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented 
+++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.php
@@ -2,6 +2,8 @@
+@trigger_error('The ' . __NAMESPACE__ . '\QueryFactory class is deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityStorageInterface::getQuery() or \Drupal\Core\Entity\EntityStorageInterface::getAggregateQuery() instead. See https://www.drupal.org/node/2849874.', E_USER_DEPRECATED);
+

Didn't we stop doing this in the class directly and move this to the constructor?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@tstoeckler that's for things that are scanned like plugins.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Ahh, I didn't know that. Thanks for the clarification. Looks great!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 6f8f239 and pushed to 8.6.x. Thanks!

  • catch committed 6f8f239 on 8.6.x 
    Issue #2968519 by alexpott, amateescu, martin107: The entity.query...

Status: Fixed » Closed (fixed)

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