Deprecate uri_callback in routes for entities

Um.

OK, so the second paragraph does say that you have the option of providing the routes in a routing.yml file, or using a route handler class. That is correct -- you can do either one. Node, and I think all the rest of the core entities, use route handlers.... so I think the 2nd paragraph is fine.

So... it looks like just the first paragraph is wrong. You don't have to use uri_callback any more, and in fact it seems to be kind of deprecated.

It looks like it was removed as part of #2010184: [meta] convert ‘uri_callback’ entities param to EntityInterface::uri() method or a related issue, but the docs were unfortunately not updated and there is also not a change notice. :(((((

I think we should make this a child issue of that other one. I'll comment over there too. Not having a change record and having docs like this is Not Really OK.

An attempt at improving this. It does appear that uri_callback is effectively deprecated now, though it is still used in some edge cases.

Thanks! Most of this seems like a great improvement.

One thing I found confusing though:

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -714,7 +713,8 @@ function hook_entity_view_mode_info_alter(&$view_modes) {
  *   - uri_callback: The same as the 'uri_callback' key defined for the entity
  *     type in the EntityManager, but for the bundle only. When determining
  *     the URI of an entity, if a 'uri_callback' is defined for both the
- *     entity type and the bundle, the one for the bundle is used.
+ *     entity type and the bundle, and the one for the entity type would
+ *     normally be used, the one for the bundle is used instead.

What does this really mean, about using the uri callback for the bundle? I don't understand it. Actually, I don't understand the previous docs either, but the added text 'and the one for the entity type would normally be used' makes it even more confusing, in my opinion.

Let's try a simpler approach to that section.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -711,10 +710,8 @@ function hook_entity_view_mode_info_alter(&$view_modes) {
+ *   - uri_callback: (optional) Overrides the 'uri_callback' key defined in the
+ *     entity type annotation, for the bundle only.

Ah, that is definitely way more clear!!

Nitpick: maybe replace "the" with "this" at the end? "... for this bundle only".

Yep, that reads better.

OK, looks good to me, thanks!

Random fail in Drupal\views_ui\Tests\OverrideDisplaysTest, back to RTBC

Thanks for working on this.

I think we should officially deprecate uri_callback and this documentation should clarify that a) it is deprecated and b) it only works when *not* using link templates. I even wondered if we should go as far as removing any documentation about it. The only reason to not remove the code right now is because that would be an API change and we have in fact one working implementation of it in core. So we could even argue that we shouldn't document a feature that you're not supposed to use.

As mentioned in other issues, the definition on the term entity is bogus, it's not used because it has link templates. The only case where it is in fact used is aggregator items. And there it's IMHO bad usage because it's really the URL to the external source, *not* the entity in Drupal. We should have just added a getSourceUrl() or so method. Similar for the file entity, which does an even weirder thing by overwriting the url() method (which nobody thought to update when toUrl() was added).

Deprecating it seems like a good idea. In which case this is not just a Documentation issue any more.

Added deprecation warnings to the interface and member variable, and reworded the existing uses to make the warning even stronger.

I doubt this will get into 8.0.x, so we should probably say deprecated in 8.1.x

Or more likely, 8.2.

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -189,6 +189,9 @@ class EntityType implements EntityTypeInterface {
   +   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
   

   Also I think most of our deprecation messages say something like:

   @deprecated in 8.1.0...

   not

   ... in Drupal 8.0.x-dev ...

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -189,6 +189,9 @@ class EntityType implements EntityTypeInterface {
   +   * @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0.
   +   *   Use Drupal\Core\Entity\EntityInterface::toUrl() for URL generation.
       */
      protected $uri_callback = NULL;
   

   Really though... I don't think that saying to use toUrl() for URL generation is all that relevant here. The $uri_callback() was more of a substitute for links annotation, from a developer perspective (the developer who is creating the entity).

   So ... how about also saying to define the URLs for entities using the links annotation or a route handler, and maybe linking to the entity API topic for more information?

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -189,6 +189,9 @@ class EntityType implements EntityTypeInterface {
   +   *   Use Drupal\Core\Entity\EntityInterface::toUrl() for URL generation.
   

   Whenever you have a class with namespace in docs, start it with \

I cheated by basically copying the deprecation notice from label_callback, hopefully improved in this version.

This came up as a random triage bug in #bugsmash.

I think is more of a task than a bug if it is just something we want to deprecate, however we can only deprecate critical things in 9.5.x for removal in 10.0.x, so postponing until 10.1.x now.

No longer postponed

I think we need to detect when the key is set when collecting entity info and trigger a deprecation then. If we did that we'd get errors for comment and forum. #2010202: Deprecate comment_uri().

Moved the patch to an MR, added deprecation notices on getUriCallback() and setUriCallback(), maybe we need more?

Deprecated some more.

Coming here from #3301682-46: Define bundle classes via attributes, where I discovered that in the conversion from annotations to attributes, the uri_callback parameter was typed as string|null , which made it impossible to specify class methods as the uri_callback, and that since that issue has never come up, pretty reasonable to conclude that it's almost never used.

Anyway, MR looks good other needing a CR URL in all the deprecation messages. Good for RTBC once the CR is created and URLs are updated.

Actually, caught one documentation reference in entity.api.php for hook_entity_bundle_info() that probably should have a (deprecated) note on there somewhere. So back to NW for that and the CR.

Added a CR, updated all links. Also decided just to remove that reference in the API docs as nobody should be relying on uri_callback being returned any more anyway.

Didn't notice before, but the MR target branch is 11.x, not main, so there are 1000 file changes showing.

MR and CR lgtm.

Committed/pushed to main and 11.x, thanks!