Deprecate entity.query service and replace with using the entity storage's getQuery() method

Removing the needs beta evaluation tag as its needed anymore. Could we mark it as deprecated in patch and will be removed in 9.0.x contrib modules might be using it ? if yes then we need a patch for it

Would be good to see before & after example code.

That's easy:

\Drupal::entityQuery('node')->... || $container->get('entity.query')->get($entity_type)

vs.

\Drupal::entityTypeManager()->getStorage('node')->getQuery()->..

Yes the second is a bit longer, but the point is that when you work with entity query, you also need the storage in 95%+ of the cases to actually load the entities you queried and then you need the storage anyway. So most code will look like this then:

$storage = \Drupal::entityTypeManager()->getStorage('node'); (or properly injected)

$ids = $storage->getQuery()->...

$storage->load($ids);

The fun thing is that QueryFactory already calls the storage but on the entity manager. So this is kind of already deprecated, this just makes it official. And we save a bunch of method calls by calling it directly.

And a ton of injections, as written above, basically every single example that uses the query factory already has the storage injected, as you need that to do anything with the results anyway. This doesn't convert everything yet, just a part of it.

Killed another 200 lines of code and fixed the failing test. total summary is now:

36 files changed, 181 insertions(+), 523 deletions(-)

The only usages that aren't converted now are in entity query tests, there are like a million usages there, converting probably makes sense as a separate patch.

Oops.

Fixing book module tests.

2 files changed, 2 insertions(+), 2 deletions(-)

Fixed all the failures.
The patch is doing changes in services of book and forum module.Should we mark them as internal in this patch or do in a followup?

See the discussion in #2624770: Use more specific entity.manager services in core.services.yml in regards to constructor arguments and protected properties, those are implicitly @internal. And I think it's fairly unlikely that those services are overridden...

+++ b/core/modules/forum/forum.services.yml
@@ -24,5 +24,5 @@ services:
     class: Drupal\forum\ForumUninstallValidator
     tags:
       - { name: module_install.uninstall_validator }
-    arguments: ['@entity.manager', '@entity.query', '@config.factory', '@string_translation']
+    arguments: ['@entity.manager', '@config.factory', '@string_translation']
     lazy: true

Still using entity manager here...

Nice work. Some small fixes.

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryFactory.php
   @@ -14,8 +14,9 @@
   + * @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.
     */
    class QueryFactory implements ContainerAwareInterface {
   

   Shouldn't we @trigger_error("...", E_USER_DEPRECATED) in both, ::get() and ::getAggregate()?

2. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   index 7380e1daec..fb7dd11026 100644
   --- a/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   
   --- a/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   +++ b/core/modules/aggregator/src/Plugin/Block/AggregatorFeedBlock.php
   

   Unused statement: use Drupal\Core\Entity\Query\QueryInterface;

3. +++ b/core/modules/block/src/Tests/NewDefaultThemeBlocksTest.php
   @@ -46,10 +46,10 @@ function testNewDefaultThemeBlocks() {
   +    $default_block_names = $this->container->get('entity_type.manager')->getStorage('block')->getQuery()
   ...
   +    $new_blocks = $this->container->get('entity_type.manager')->getStorage('block')->getQuery()
   
   @@ -64,7 +64,7 @@ function testNewDefaultThemeBlocks() {
   +    $new_blocks = $this->container->get('entity_type.manager')->getStorage('block')->getQuery()
   

   Let's save $this->container->get('entity_type.manager')->getStorage('block') in a $storage var and reuse it.

4. +++ b/core/modules/block_content/src/Form/BlockContentTypeDeleteForm.php
   @@ -13,36 +11,10 @@
   +    $blocks = $this->entityTypeManager->getStorage('block_content')->getQuery()->condition('type', $this->entity->id())->execute();
   

   Let's add a line break after ->getQuery(). It's hard to read.

5. +++ b/core/modules/comment/src/Form/CommentTypeDeleteForm.php
   @@ -86,7 +74,11 @@ public static function create(ContainerInterface $container) {
   +    $comments = $this->entityTypeManager
   +      ->getStorage('comment')
   +      ->getQuery()
   +      ->condition('comment_type', $this->entity->id())
   +      ->execute();
   

   ->getStorage()->getQuery() can go on the first line so we have the query object on the upper line.

   It would be nice if we can format in this way all occurrences that we touch, with the query object on the first line followed by ->condition() on 2nd line.

6. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   @@ -27,13 +26,6 @@ class EntityComment extends EntityContentBase {
   -  protected $entityQuery;
   

   Interesting, this was not used at all.

7. +++ b/core/modules/system/src/Controller/SystemController.php
   @@ -20,13 +19,6 @@
   -  protected $queryFactory;
   

   Unused too :)

8. +++ b/core/modules/views/src/Views.php
   @@ -213,13 +213,13 @@ public static function getApplicableViews($type) {
   -    foreach (\Drupal::entityManager()->getStorage('view')->loadMultiple($entity_ids) as $view) {
   +    foreach (\Drupal::entityTypeManager()->getStorage('view')->loadMultiple($entity_ids) as $view) {
   

   Unrelated.

9. +++ b/core/tests/Drupal/Tests/Core/DrupalTest.php
   @@ -306,6 +326,7 @@ public function testTypedDataManager() {
      public function testToken() {
   +
        $this->setMockContainerService('token');
   

   Unrelated.

EDIT: @almaudoh, we are not changing the entity manager in this patch.

Also, because we're deprecating here a service, we need a change note. Let's not forget also the IS update.

Reroll

#20: Changed. I actually changed the constructor there already because I was changing those lines anyway.

#21

1. Not sure, maybe. We still haven't got rid of all calls in core and unlike other places where we added it, I expect that this is used *a lot* in contrib and custom code. We could maybe do it as part of the issue to remove all remaining test-usages as well?
2. Fixed
3. Fixed
4. Fixed
5. I'm not sure about that rule (AFAIK our coding standards says to indent all calls) but changed for now.
6. & 7. Yeah, probably was removed at some point when refactoring things
8. Damn, thought I could sneak that in ;)
9. Fixed.

Also opened #2849873: Replaces usages of entity.query service in entity query tests.

Change record: https://www.drupal.org/node/2849874

Actually, the reason I switch there is that entityQuery() uses entity_type.manager, that means keep using entityManager() would require us to mock both entity.manager and entity_type.manager, I think this is easier in that case. Reverted that.

That makes sense. I see #21 was addressed. Looks good for me.

Needs a re-roll afaict.

Rerolled.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

The 8.3.x commit message didn't come through, and the "fixed" didn't take...

But the commit exists in the codebase. Seems that d.o failed to register it.

This happened on a couple of issues today fwiw..

d.o also ignored it when @catch changed the branch. Weird... The commit is definitely there as others have said.

I ran across this because removing net 12 lines from FieldStorageAddForm.php caused git apply to switch two hunks in #2847685: Update doc blocks for configureEntityFormDisplay() and configureEntityViewDisplay().

I have a couple of questions about the change record.

For services, forms and controllers, change $this->entityQueryFactory->get('node') to $this->entityTypeManager->getStorage('node')->getQuery() or ...

I am still learning all this stuff, so I may be unclear on the distinction is between entityTypeManager and entityManager, but I see this as part of the patch, so I wonder if that is a typo in the change record:

-    $field_ids = $this->queryFactory->get('field_config')
+    $field_ids = $this->entityManager->getStorage('field_config')->getQuery()

Maybe the change record could be expanded to explain more fully how to update existing code.

The other question is whether the change record should document the changes to the public API. In fact, I am surprised that this is allowed in a minor version update. Is it safe to assume that the constructor is never called except by the dependency injection system? In the file I am looking at, I see

@@ -67,15 +59,12 @@ class FieldStorageAddForm extends FormBase {
    *   The entity manager.
    * @param \Drupal\Core\Field\FieldTypePluginManagerInterface $field_type_plugin_manager
    *   The field type plugin manager.
-   * @param \Drupal\Core\Entity\Query\QueryFactory $query_factory
-   *   The entity query factory.
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The configuration factory.
    */
-  public function __construct(EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_plugin_manager, QueryFactory $query_factory, ConfigFactoryInterface $config_factory) {
+  public function __construct(EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_plugin_manager, ConfigFactoryInterface $config_factory) {

Removing a parameter from a public function is a change to the API, right?

Constructors, especially in forms and controllers are excluded from BC: https://www.drupal.org/core/d8-bc-policy

entityManager is deprecated in favor of entityTypeManager, some classes only had that available which is why I sticked to that to avoid making too many changes.

@Berdir, thanks for the pointer and the explanation!

Constructors, especially in forms and controllers are excluded from BC: https://www.drupal.org/core/d8-bc-policy

These are objects where developers are not expected to call the constructor directly in the first place.

That's annoying. I won't rely on Core code in the future and duplicate all of it so it's backwards-compatible for Drupal 8. :(

DefaultMenuLinkTreeManipulators was nice to override as I didn't actually have to write code for it - just pass in the correct parameters. Now I need to write a BC shim for it or create a new BC-breaking release in my module.

I wrote a blogpost recently on how this can be done safely at least for services: http://www.md-systems.ch/blog/techblog/2016/12/17/how-safely-inject-addi...

Thanks, Berdir. That makes sense for the situation when swapping classes.

I have a class though that only extends the default manipulator class so any service that I make needs to adhere to the constructor/API. Though similarly could write a service provider that detects what dependencies core is currently using, and then change my service appropriately. This API change is still disappointing and annoying to me despite the improvement of using EntityTypeManager (and it is the definition of an API change no matter what a policy dictates).

Edit: actually these approaches are still bad because you can't do the same logic for unit tests so unit tests will be broken for various versions of Drupal.

You might be able to doing it by adding a parent: name_of_core_service instead of defining the arguments yourself. But yes, doesn't work at least for unit tests.

I've been affected and annoyed by this as well. In commonly extended classes and when adding constructor arguments, we often add them as optional with a fallback, but when removing/renaming them, it's not so easy. But if we wouldn't be allowed to do this, it would be impossible for core to remove its own deprecated usages and would make improvements/new features in minor releases a lot harder and resulting in a lot of ugly, non-injected, non-testable code.