Replace entity_query() with Drupal::entityQuery() [#1957148]

Comment	File	Size	Author
#23	1957148-23.patch	32.18 KB	damiankloip
#12	1957148-12.patch	30.59 KB	damiankloip
#8	1957148-8.patch	30.99 KB	damiankloip
#8	interdiff.txt	763 bytes	damiankloip
#6	1957148.patch	30.99 KB	damiankloip

Comment #1

berdir

German

Switzerland

commented 30 March 2013 at 20:37

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

Log in or register to post comments

Comment #2

dawehner

German

commented 30 March 2013 at 21:07

What should we do with entity_query_aggregate()?

Log in or register to post comments

Comment #3

berdir

German

Switzerland

commented 30 March 2013 at 21:10

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

Log in or register to post comments

Comment #4

dawehner

German

commented 30 March 2013 at 21:50

Good question!

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

Log in or register to post comments

Comment #5

xjm

she/her

English

commented 31 March 2013 at 05:35

Status:

Active

» Postponed

Postponing on the main issue.

Log in or register to post comments

Status	File	Size
new	1957148.patch	30.99 KB

Comment #7

berdir

German

Switzerland

commented 3 April 2013 at 11:16

+++ 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".

Log in or register to post comments

Comment #8

damiankloip commented 3 April 2013 at 11:30

Status	File	Size
new	interdiff.txt	763 bytes
new	1957148-8.patch	30.99 KB

Changed that comment.

Log in or register to post comments

Comment #9

dawehner

German

commented 3 April 2013 at 19:24

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

Log in or register to post comments

Comment #10

damiankloip commented 3 April 2013 at 20:43

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

Log in or register to post comments

Comment #11

dawehner

German

commented 4 April 2013 at 00:15

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.

Log in or register to post comments

Comment #12

damiankloip commented 4 April 2013 at 06:38

Status	File	Size
new	1957148-12.patch	30.59 KB

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.

Log in or register to post comments

Comment #13

ParisLiakos commented 4 April 2013 at 19:31

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

Log in or register to post comments

Comment #14

damiankloip commented 4 April 2013 at 19:43

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

Log in or register to post comments

Comment #15

ParisLiakos commented 4 April 2013 at 19:45

Status:

Needs work

» Reviewed & tested by the community

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

Log in or register to post comments

Comment #16

chx commented 4 April 2013 at 20:27

Sure, go ahead...

Log in or register to post comments

Comment #17

webchick

she/they

English

Vancouver 🇨🇦

commented 7 April 2013 at 20:08

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?

Log in or register to post comments

Comment #18

xjm

she/her

English

commented 7 April 2013 at 20:11

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

Log in or register to post comments

Comment #19

damiankloip commented 7 April 2013 at 20:21

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

Log in or register to post comments

Comment #20

berdir

German

Switzerland

commented 7 April 2013 at 20:26

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?

Log in or register to post comments

Comment #21

Crell commented 7 April 2013 at 20:32

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.

Log in or register to post comments

Comment #22

7 April 2013 at 23:48

Status:

Needs review

» Needs work

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

Log in or register to post comments

Comment #23

damiankloip commented 8 April 2013 at 06:49

Status:

Needs work

» Needs review

Status	File	Size
new	1957148-23.patch	32.18 KB

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.

Log in or register to post comments

Comment #24

chx commented 8 April 2013 at 08:04

Status:

Needs review

» Reviewed & tested by the community

Log in or register to post comments

Comment #25

alexpott

he/they

English

🇪🇺🌍

commented 11 April 2013 at 12:57

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.

Log in or register to post comments

Comment #26

damiankloip commented 11 April 2013 at 17:11

Status:

Active

» Needs review

Updated these:

http://drupal.org/node/1827278
http://drupal.org/node/1918702
http://drupal.org/node/1902034
http://drupal.org/node/1882418

Log in or register to post comments

Comment #27

berdir

German

Switzerland

commented 11 April 2013 at 18:32

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.

Log in or register to post comments

Comment #28

berdir

German

Switzerland

commented 11 April 2013 at 18:51

Issue tags:

-Needs change record

Removing tag.

Log in or register to post comments

Comment #29

andypost

he/him

Russian

commented 19 April 2013 at 11:59

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

Log in or register to post comments

Comment #30

jibran

he/him

Urdu

Toronto, Canada

commented 19 April 2013 at 12:12

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?

Log in or register to post comments

Comment #31

andypost

he/him

Russian

commented 19 April 2013 at 13:31

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

Log in or register to post comments

Comment #32

damiankloip commented 19 April 2013 at 14:26

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?