Follow-up of #1937600: Determine what services to register in the new Drupal class

Replace all calls to entity_query() with Drupal::entityQuery() and then remove the entity_query() function.

Files: 
CommentFileSizeAuthor
#23 1957148-23.patch32.18 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 54,072 pass(es).
[ View ]
#12 1957148-12.patch30.59 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 53,487 pass(es), 9 fail(s), and 6 exception(s).
[ View ]
#8 1957148-8.patch30.99 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,993 pass(es).
[ View ]
#8 interdiff.txt763 bytesdamiankloip
#6 1957148.patch30.99 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 54,015 pass(es).
[ View ]

Comments

Berdir’s picture

Issue tags:+Novice

Instructions:

Search & replace all calls to entity_query() with the mentioned method. Note that calls within namespaced classes (have a namespace line and are in the lib/ folder) needs to be "\Drupal".

dawehner’s picture

What should we do with entity_query_aggregate()?

Berdir’s picture

Good question. That function doesn't seem to be used anywhere?

dawehner’s picture

Good question!

No, not at all. Let's drop that in this issue.

xjm’s picture

Status:Active» Postponed

Postponing on the main issue.

damiankloip’s picture

Status:Postponed» Needs review
StatusFileSize
new30.99 KB
PASSED: [[SimpleTest]]: [MySQL] 54,015 pass(es).
[ View ]
Berdir’s picture

+++ b/core/modules/tour/tour.moduleundefined
@@ -118,7 +118,7 @@ function tour_preprocess_page(&$variables) {
-    // @todo replace this with an entity_query() that does path matching when
+    // @todo replace this with a Drupal::entityQuery() that does path matching when

I guess this should either not use "a" or something like "an entity query".

damiankloip’s picture

StatusFileSize
new763 bytes
new30.99 KB
PASSED: [[SimpleTest]]: [MySQL] 53,993 pass(es).
[ View ]

Changed that comment.

dawehner’s picture

+++ b/core/includes/entity.incundefined
@@ -748,7 +731,7 @@ function entity_query($entity_type, $conjunction = 'AND') {
function entity_query_aggregate($entity_type, $conjunction = 'AND') {
-  return drupal_container()->get('entity.query')->getAggregate($entity_type, $conjunction);
+  return Drupal::service('entity.query')->getAggregate($entity_type, $conjunction);

As written before, I think we should get rid of this wrapper function.

damiankloip’s picture

I think maybe it's better if we deal with entity_query_aggregate() in a follow up?

dawehner’s picture

There is noone using this procedural function at the moment, so the longer it sticks in, the more people might use it, even they shouldn't,
but yeah I don't really care.

damiankloip’s picture

StatusFileSize
new30.59 KB
FAILED: [[SimpleTest]]: [MySQL] 53,487 pass(es), 9 fail(s), and 6 exception(s).
[ View ]

I have removed any changes to entity_query_aggregate and created another issue for that, #1960882: Remove entity_query_aggregate function with a Drupal class method.

ParisLiakos’s picture

Status:Needs review» Needs work
core/modules/field/lib/Drupal/field/Tests/BulkDeleteTest.php:    $factory = \Drupal::service('entity.query');
core/modules/field/field.module:  $factory = Drupal::service('entity.query');
core/modules/field/field.crud.inc:  $factory = Drupal::service('entity.query');
core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryTest.php:    $this->factory = \Drupal::service('entity.query');
core/modules/system/lib/Drupal/system/Tests/Entity/EntityQueryRelationshipTest.php:    $this->factory = \Drupal::service('entity.query');

lets replace those too

damiankloip’s picture

Not sure, we can't use entityQuery() if we want the factory?

ParisLiakos’s picture

Status:Needs work» Reviewed & tested by the community

uh oh, sorry totally missed the argument is missing:P
RTBC!

chx’s picture

Sure, go ahead...

webchick’s picture

Status:Reviewed & tested by the community» Needs review

I really don't think this belongs on the Drupal class. it feels like it belongs on an Entity class instead. Thoughts?

xjm’s picture

#12: 1957148-12.patch queued for re-testing.

damiankloip’s picture

It seems the issue in the summary was the place to be discussing that :)

Berdir’s picture

Issue tags:-Novice

We discussed that in IRC too at some point. I generally agree, but there are some questions that need to be answered to be able to do that.

- How should that class be named and in which namespace should it live? We already have Drupal\Core\Entity\Entity which is the base class for all entity types, we'd have to rename that to EntityBase or EntityTypeBase first if we want to use that. @msonnabaum also suggested just Drupal\Core\Entity to avoid duplicating the namespace and classnames but I'm not sure about placing classes directly in Drupal\Core. We already have quite a few but it's not a component on its own so it somehow feels wrong. @Crell?

- We also have the EntityManager, which is the entity type plugin manager and e.g. responsible for returning entity controllers/handlers and later on possibly also creating of new entities and entity field definitions. Might not be trivial to separate these two and explain the difference between them?

Crell’s picture

The classes in \Drupal\Core directly fall into 2 categories:
1) They're so foundational that they don't have a "sub" system.
2) We haven't gotten around to putting them into a subsystem yet but should. (All of the Controller stuff is in that category.)

The entity system is definitely a subsystem of its own, so it doesn't count as class 1.

Per Angie's comment in #17, at least this week the Entity system is in /core/lib, so it's part of the core framework toolchain. That means \Drupal:: is a reasonable place to put its legacy service wrapper, IMO. It would only need an \Entity:: static if it were in a disable-able module.

Status:Needs review» Needs work

The last submitted patch, 1957148-12.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new32.18 KB
PASSED: [[SimpleTest]]: [MySQL] 54,072 pass(es).
[ View ]

As per webchick's request in #1960882: Remove entity_query_aggregate function with a Drupal class method. Adding the entity query aggregate changes here too.

chx’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Title:Replace entity_query() with Drupal::entityQuery()» Change notice: Replace entity_query() with Drupal::entityQuery()
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed bea3588 and pushed to 8.x. Thanks!

Think this might need a change notice.

damiankloip’s picture

Berdir’s picture

Title:Change notice: Replace entity_query() with Drupal::entityQuery()» Replace entity_query() with Drupal::entityQuery()
Priority:Critical» Normal
Status:Needs review» Fixed

Updated change notices look good to me.

Berdir’s picture

Issue tags:-Needs change record

Removing tag.

andypost’s picture

Title:Replace entity_query() with Drupal::entityQuery()» Change notice: Replace entity_query() with Drupal::entityQuery()
Priority:Normal» Critical
Status:Fixed» Needs work

This needs change notice

jibran’s picture

Status:Needs work» Needs review

All the change notification were updated in #26. Issue link is also added now. Do you want separate change notification for this issue?

andypost’s picture

Title:Change notice: Replace entity_query() with Drupal::entityQuery()» Replace entity_query() with Drupal::entityQuery()
Priority:Critical» Normal
Status:Needs review» Reviewed & tested by the community

Seems update is enough, but we need a common way to alert other issues with simmilar changes

damiankloip’s picture

Status:Reviewed & tested by the community» Fixed

That sounds like a messy way to go down, and maybe not worth the effort? I would say the amount of existing change notices that need updating in this way is a little more rare.

@andypost, do you want to create a followup to discuss that idea further?

andypost’s picture

andypost’s picture

Issue tags:+API change

Tagged

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