Add docs to *.services.yml to tell that @entity.manager is deprecated

Deprecating something is not really a Documentation issue (it's a core maintainer decision to deprecate something, not just a matter of making sure something is documented accurately), so I am moving this over to the Entity component.

And by the way, the docs look OK to me... however I am not sure that anyone will notice them there, and I know that they will not be shown on api.drupal.org on the docs page for the entity.manager service, because it doesn't parse # comments in YML files at all.

So.

My feeling is that this should be documented somewhere else in addition:
- Maybe on the class Drupal\Core\Entity\EntityManager it could be mentioned? And is that class going away too?
- Is there a Drupal:: method that returns the entity manager? If so it should definitely be deprecated as well.

OK. Still should be Needs Work based on #8. The documentation added here is not going to be noticed.

So someone just filed #2641704: Suggest alternative to EntityManager::getStorage, which I've marked as a duplicate of this issue. It illustrates what I said in #8 quite nicely -- so yes, EntityManager is deprecated but unfortunately whoever did that did not tell what to do as an alternative. This information needs to be added to the EntityManager class, and all of its methods probably.

Wow, thanks for all the work that went into this patch! A great start.

I think there are a couple of things that need to be improved before we can commit it:

1. +++ b/core/core.services.yml
   @@ -494,6 +494,28 @@ services:
        arguments: ['@theme_handler', '@config.factory', '@config.installer', '@module_handler', '@config.manager', '@asset.css.collection_optimizer', '@router.builder', '@logger.channel.default', '@state']
   +  # @deprecated in 8.0.x, will be removed before 9.0.0. The 'entity.manager'
   

   I'm wondering if we should perhaps add a blank line before this comment? We do not have comments in front of services anywhere else in Core that I am aware of... it will at least set this off a bit!

2. +++ b/core/core.services.yml
   @@ -494,6 +494,28 @@ services:
   +  #   service was split in multiple services that should be used in contrib and
   

   split in => split into

3. +++ b/core/core.services.yml
   @@ -494,6 +494,28 @@ services:
   +  #   service was split in multiple services that should be used in contrib and
   +  #   custom modules:
   

   how about just saying:

   ... that should be used instead of this service:

   (not say "in contrib and custom modules")

4. +++ b/core/core.services.yml
   @@ -494,6 +494,28 @@ services:
   +  #   - entity_type.manager: Provides an interface to entity type managers.
   

   I'm a bit confused about why we're documenting services as "providing an interface" ??? These don't make sense to me.

5. +++ b/core/core.services.yml
   @@ -494,6 +494,28 @@ services:
   +  #   - entity.repository: Provides an interface for an entity repository.
   +  #   - entity_display.repository: Provides an interface for an entity display
   +  #     repository.
   +  #   - entity_field.manager: Provides an interface for an entity field manager.
   

   It looks like these lines got taken from some interface one-line descriptions. Really I don't think this is the best way to document what the services do.

6. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -15,7 +15,31 @@
   + *   EntityManager service class was split in multiple classes that should be
   + *   used in contrib and custom modules:
   

   see above notes about "in contrib and custom modules" and "split in"

7. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -15,7 +15,31 @@
   + *   - \Drupal\Core\Entity\EntityTypeManager: Provides an interface to entity
   + *     type managers.
   

   Um... This is a class, not an interface? Strange documentation.

8. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -15,7 +15,31 @@
   + *   - \Drupal\Core\Entity\EntityRepository: Provides an interface for an entity
   ...
   + *   - \Drupal\Core\Entity\EntityDisplayRepository: Provides an interface for an
   

   interface? Please check the rest of these.

9. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -27,7 +51,10 @@ class EntityManager implements EntityManagerInterface, ContainerAwareInterface {
   +   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. Use
   +   *   \Drupal\Core\Entity\EntityTypeManager::clearCachedDefinitions() instead.
   

   So... Really people cannot use this method directly. They need to get the service and call a method on it. I am not sure this is the best way to document what people should be using.

   I would suggest something like:

   Use the clearCachedDefinitions() method on the 'entity_type.manager' service instead.

10. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -27,7 +51,10 @@ class EntityManager implements EntityManagerInterface, ContainerAwareInterface {
    +   * @see \Drupal\Core\Entity\EntityTypeManager::clearCachedDefinitions()
    

    The @see makes sense to me though, since it would lead to docs about the method they would call.

    But shouldn't it be linking to an interface method instead of a particular class's implementation of it? If possible it should (if the interface exists).

11. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -43,7 +70,10 @@ public function clearCachedDefinitions() {
    +   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. Use
    +   *   \Drupal\Core\Entity\EntityTypeManager::getDefinition() instead.
    

    See comment about the clearCachedDefinitions() method. Applies to this one too, and all the rest.

12. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -43,7 +70,10 @@ public function clearCachedDefinitions() {
    +   * @see \Drupal\Core\Entity\EntityTypeManager::getDefinition()
    

    Comment above about @see also applies here, and to all the rest of the methods.

13. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
    @@ -20,14 +20,19 @@
    +   *
        */
       public function getLastInstalledDefinition($entity_type_id);
    

    This one is missing the @see and/or has an extra blank line in the docs.

14. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
    @@ -20,14 +20,19 @@
    +   *   instead.
        */
       public function getLastInstalledFieldStorageDefinitions($entity_type_id);
    

    Also missing the @see.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -15,7 +15,31 @@
   + *   - \Drupal\Core\Entity\EntityTypeManager: Provides an interface to entity
   ...
   + *   - \Drupal\Core\Entity\EntityRepository: Provides an interface for an entity
   ...
   + *   - \Drupal\Core\Entity\EntityDisplayRepository: Provides an interface for an
   ...
   + *   - \Drupal\Core\Entity\EntityFieldManager: Provides an interface for an
   

   These are not true, either point to the interface, or give it a concrete definition.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -473,7 +667,11 @@ public function useCaches($use_caches = FALSE) {
   +   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. Use
   +   *   \Drupal\Core\Entity\EntityLastInstalledSchemaRepository::getLastInstalledFieldStorageDefinitions()
   +   *   instead.
   +   *
   +   * @see \Drupal\Core\Entity\EntityLastInstalledSchemaRepository::getLastInstalledFieldStorageDefinitions()
   

   Okay, so it's more helpful than before, but I find this redundant. Either add it to the @deprecated, or use @see.

   Also we usually point to interfaces.

Thanks! This is definitely looking better.

A few things still to fix:

1. +++ b/core/core.services.yml
   @@ -494,6 +494,30 @@ services:
   +  #   - entity_type.repository: Provides additional methods for loading entity
   +  #     types or other data from entity types.
   

   ... for loading entity types ... from entity types....

   What does this mean? How can you load an entity type from an entity type?

2. +++ b/core/core.services.yml
   @@ -494,6 +494,30 @@ services:
   +  #     creation, update and deletion.
   

   Needs comma before "and"

3. +++ b/core/core.services.yml
   @@ -494,6 +494,30 @@ services:
   +  #     field storage definition creation, update and deletion.
   

   needs comma before "and"

4. +++ b/core/core.services.yml
   @@ -494,6 +494,30 @@ services:
   +  #     definition creation, update and deletion.
   

   comma before "and"

5. One more from core.services.yml:
   

   +  #   - entity_display.repository: Provides additional methods for loading
   +  #     entity display modes.
   

   This interface also handles entity form modes.

6. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -15,7 +15,33 @@
   + * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. The
   + *   EntityManager service class was split into multiple classes that should be
   + *   used instead of this service:
   

   The list items in this section... see comments on the core.services.yml file above; all apply here as well.

7. Also, I think in EntityManagerInterface, rather than having @see separated from @deprecated, we should be combining the reference into the @deprecated and removing the @see. I don't see why we need an @see and every @deprecated is supposed to tell you what to use instead. As it is, the @deprecated doesn't tell you this.

Good point on #17-5 on display modes. I've never liked the terminology -- to me "display" is a synonym of "view", but that is not your fault. ;)

Regarding @deprecated, see
https://www.drupal.org/node/1354#deprecated
So the docs about replacement should be in each @deprecated tag docs itself, not in a separate @see section, even in the methods.... noted below.

1. +++ b/core/core.services.yml
   @@ -494,6 +494,30 @@ services:
   +  #     types or retrieving entity types related data.
   

   "entity types related data" ... This is awkward wording/punctuation. It needs to be either

   entity-type-related data
   or
   data about entity types
   or maybe just

   entity type data

   Yes, I think that last choice would be the best, really. Sorry I didn't notice this in the last review.

   And don't forget to update the class docs too.

2. +++ b/core/core.services.yml
   @@ -494,6 +494,30 @@ services:
   +  #   - entity_entitytype.listener: Provides methods for reacting on entity type
   +  #     creation, update, and deletion.
   +  #   - entity_bundle.listener: Provides methods for reacting on entity bundle
   

   reacting on -> reacting to

   This applies to a bunch of spots in the docs, everywhere it says "reacting on". Sorry I didn't notice this in the last review.

   And don't forget to update the class docs too.

3. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -27,7 +53,11 @@ class EntityManager implements EntityManagerInterface, ContainerAwareInterface {
      /**
       * {@inheritdoc}
       *
   -   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   +   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0. Use the
   +   *   clearCachedDefinitions() method on the 'entity_type.manager' service
   +   *   instead.
   +   *
   +   * @see \Drupal\Core\Entity\EntityTypeManagerInterface::clearCachedDefinitions()
       */
      public function clearCachedDefinitions() {
   

   Oh gracious.

   I just realized that all of these @deprecated and @see tags are in with @inheritdoc.

   You either need to have @inheritdoc by itself, or you need to have a proper docs header. This violates our standards and does not work on api.drupal.org -- it doesn't pick up the inherited docs. Check this method as it is now and you'll see this. And if you want to discuss the standard, the issue for that is #1994890: Allow {@inheritdoc} and additional documentation; PLEASE do not discuss the standard here. We just need to follow it for now.

   So this needs to be fixed.

   Probably wherever it is inheriting the docs from is where the @deprecated statement needs to be? Otherwise you'll need to copy the docs from there into here.

   Applies to all of the methods.

4. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -13,21 +13,27 @@
       * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   +   *
   +   * @see \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface::getLastInstalledDefinition()
   

   This does need to have the replacement documented in the @deprecated tag. Please.

5. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -13,21 +13,27 @@
       * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   +   *
   +   * @see \Drupal\Core\Entity\EntityLastInstalledSchemaRepositoryInterface::getLastInstalledFieldStorageDefinitions()
       */
   

   Here too.

OK, that looks better.

So the question remains: should we be putting the @deprecated on the class method, interface method, or both?

Sorry, I think my question was not understood.

Let's step back a moment.

We are deprecating (or rather have deprecated) the EntityManager class and all of its methods. In core currently, the EntityManager class and most of the methods already have @deprecated tags in their docs headers. However, they look like this:

  /**
   * {@inheritdoc}
   *
   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   */
  public function clearCachedDefinitions() {

which is not allowed.

The docs from these methods are intended mostly to be inherited (with additions, which is the part that doesn't work and violates our docs standards) from the corresponding EntityManagerInterface methods.

So my question is this:

Should the @deprecated tags be on the EntityManager methods (in which case they need to have full doc blocks and not try to use syntax like the above with "inheritdoc + more")? Or should the @deprecated tags be on the EntityManagerInterface methods? Or both?

Ah, right! Sorry, I missed seeing that. You even patched the EntityManagerInterface file's doc blocks. :)

I think this patch is ready to go then.

  /**
   * {@inheritdoc}
   *
   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   */
  public function clearCachedDefinitions() {

What's wrong with this from a coding standards point of view? It seems much more preferable to duplicated documentation onto a method that's been deprecated.

+++ b/core/core.services.yml
@@ -494,6 +494,30 @@ services:
+  # @deprecated in 8.0.x, will be removed before 9.0.0. The 'entity.manager'
+  #   service was split into multiple services that should be used instead of
+  #   this service:

Also I'm not at all sure about this:

- the class docs are discoverable on api.drupal.org, this isn't.
- Not having profiled it, but I'd expect Symfony's YAML parser to have to parse these comments line by line and detect the '#', so there's potentially a small cost from adding this
- Symfony 2.8/3.0 adds a deprecation framework for services, so we could use that once we've updated and figured out how to integrate it. See #2611816-20: Update to symfony 2.8 and down for some discussion of that.

There's a separate issue about "inheritdoc plus other stuff". It does not work and cannot be made to work like that, really it can't. It is also a violation of our coding standards, even though it is all over core unfortunately. See the lengthy discussion on #1994890: Allow {@inheritdoc} and additional documentation for details. I don't want to repeat the entire discussion here.

And I totally agree with you that putting the docs in .yml files is not all that great, which is why early on in this issue I insisted on getting the docs into the classes and methods too. Everyone else wanted to though... totally happy with that being overruled.

It occurred to me... maybe we could move the deprecated services to a new file, called deprecated.services.yml -- I'm not sure whether the container would recognize it, but that would be one way to proceed without adding these docs to the core.services.yml file.

In response to these two comments...

catch:

@deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.

What's wrong with this from a coding standards point of view? It seems much more preferable to duplicated documentation onto a method that's been deprecated.

claudiu.cristea:

Imagine you are writing a module and you need to use a service. Where do you go first if you need to use the EntityTypeManager but you don't remember its service ID? Does api.drupal.org help you in some way? When I'm asking myself "What is the service ID of the EntityTypeManager", I'm quickly searching the answer in core.services.yml. Or is api.drupal.org offering a quicker way to find the answer to this question?

In fact, I search api.drupal.org using Google - "site:api.drupal.org MethodImSeeking" (or even without the "site:" filter) - and based on my interactions on IRC, many people refer to the api.drupal.org pages primarily as well. It's the power of the Google. IMO, documenting the replacement method on method pages is more important than class pages, because, Google again, there's more method pages and they're more likely to be the landing page for somebody working through this.

I'm for the patch in #20, and I think this issue should moved to major, because the EntityManager is a key class in D8, and currently accounts for the largest chunk of deprecated code (not to mention undocumented deprecated code).

I'm not certain if this should remain open, with the latest patch in #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED, all methods on EntityManager now have @trigger_error(), looks like the deprecation messages have also been improved since this patch was created. We can most likely not yet add an official @deprecated tag on the service because there will likely be *some* calls to it left.

@claudiu.cristea, thanks for bring this idea up and making patches.

According to the deprecation policy for services a deprecation message can be added to a service using the 'deprecated' key.

This was for Drupal 8 which is now EOL

Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.

Thanks!