Add EntityInterface::toUrl() and EntityInterface::toLink() and mark urlInfo(), url() and link() as deprecated

Resubmitting the Patch.

The summary of this issue doesn't appear to make sense. As written, this is guaranteed to break unit tests (and Drupal in general). My understanding of this issue based on the linked document is that the intent is to:

1. Rename urlInfo() as buildUrl() in both Entity and EntityInterface (just adding it to the interface makes no sense because everything that uses it will break)
2. Rename link() as buildLink() in both Entity and EntityInterface
3. Create urlInfo() as an alias to buildUrl()
4. Create link() as an alias to buildLink()
5. Mark urlInfo(), url() and link() as deprecated in the comments, with a pointer to the appropriate replacement
6. Update the unit tests accordingly

Unfortunately life isn't quite that simple. The method name buildUrl() is already used in places derived from Entity and EntityInterface, in particular:

• \Drupal\image\EntityImageStyle
• \Drupal\image\ImageStyleInterface

This will also break anything that implements the EntityInterface API without inheriting Entity.

Given the current RC state of Drupal, this change is a major API headache.

I am removing the Novice tag, because this needs clarification on the correct course of action.

It seems to me that we have a key decision to make:

• Decide to postpone dealing with this until the next time we are allowed to break contrib, and mark the issue accordingly.
• Attempt to get this through RC target triage, in which case we have three options:
  1. Use the names urlInfo() and linkInfo(). This is not ideal, but it has the least API impact.
  2. Come up with another naming scheme for the EntityInterface of the form xxxUrl() and xxxLink(), where 'xxx' is something like 'make', 'to', 'get' or 'as'.
  3. Rename the buildUrl() in the image module, its interface and anything that calls it. There are a lot of calls to buildUrl() in Drupal, although most of them probably have nothing to do with entities.

My favoured option is 2 with a prefix of 'to'. I have checked this for API conflicts and it has none inside Drupal core.

I want to tag this rc target triage, but I think we need to make the choice outlined in #9 first.

Unless anybody objects I'm going to work on a patch based on toUrl() and toLink() whilst awaiting a decision on the correct approach. My reasoning is that toUrl() can be easily refactored by to buildUrl() with a global search and replace, but going from buildUrl() to some other xxxUrl() is a nightmare.

I have already discovered that ViewUI implements EntityInterface but without inheriting Entity...

A separate piece of work that can be done in advance of a decision is a patch to rename buildUrl() in the image module and its interface. This would allow us to locate the things that depend on that.

Here is the first patch. This creates toUrl(), toLink(). deprecates the old methods and updates the test-cases.

The files affected are:

• core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
• core/lib/Drupal/Core/Entity/Entity.php
• core/lib/Drupal/Core/Entity/EntityInterface.php
• core/modules/views_ui/src/ViewUI.php
• core/tests/Drupal/Tests/Core/Entity/EntityLinkTest.php
• core/tests/Drupal/Tests/Core/Entity/EntityUrlTest.php

In order to convert these to buildUrl() and buildLink() we can just do a search and replace. We will also need to replace ToUrl and ToLink with BuildUrl and BuildLink in method names in the unit test files.

I'm setting the status to 'Needs Review', but this is only a partial solution.

This is the patch to rename the image module buildUrl() to buildDerivativeUrl() cf createDerivative().

It also renames buildUri() to buildDerivativeUri() on the grounds that the two methods form a pair.

I hope I got most of the places where this is called from, but I still expect testing to break...

Dawehner, I agree with your comments.

Here is a new patch for adding the toUrl() and toLink() methods. toLink() now returns a Link, and the comments have been updated accordingly.

I also caught a bug (that the patch introduced) in ViewUI.

Just in case we need this information later, the testbot failures on #13 indicate the following core modules have a dependency on buildUrl() in the image module:

• rdf
• responive_image
• user

Those changes weren't spurious. The reason they were there was because those lines were confusing PHPStorm when I was trying to debug those unit tests and the things they were testing. I ended up turning them into normal comments rather than type hints. That was probably the wrong thing to do, but I didn't understand what was going on at the time.

Since then I have found some information to suggest that they are a mixture of PHPStorm syntax:

/** @var Type $variable */

and the PHP standard (also supported by PHPStorm):

/* @var $variable Type */

So what we had was type hints with syntax errors.

Now the tests and the code they are testing are working, I could change the comments back (especially the testUrlInfo() instance, since that method is essentially unchanged), but I think it could be argued that the ones in the xxxToUrl() methods are in scope and should be fixed to the PHP standard (which they aren't in either the original or the current patch).

Can someone update the issue summary with an explanation of why we break BC less than two weeks before the 8.0.0 release?

Alternatively we should think about adding this to a new interface so we can introduce the new functionality without breaking anything.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -101,6 +101,25 @@ public function bundle();
+  public function urlInfo($rel = 'canonical', array $options = array());

This is an addition to an existing interface, which means any classes implementing this interface must implement the newly added methods. As such this is a BC break. If this is an accepted break, this must be documented as such.

I think Xano's point is about implementation of interfaces. If your class implements EntityInterface but does not inherit Entity, you suddenly have an abstract class. Fortunately, if you have any automated tests this will fail testing so it is easy to spot, either in core or in contrib. Once found, this is trivial to fix.

There is a more subtle issue, which is probably higher risk of impact. That is modules that override the Url generation methods to change their behaviour (ConfigEntityBase is an example that overrides the default values for $rel and the language option). However, since nothing will currently be calling the new methods this has limited scope for causing trouble.

That is the case for low impact. We now have to make the case for why this change is important.

My understanding is that the reason for doing this is that having lots of different ways of creating URLs and links makes code harder to maintain. If we find a problem we have to fix it in lots of places. Having one recommended way of doing things plus half a dozen deprecated ways means that if an issue comes up, everybody knows that the recommended method is our priority. Since the switch to the recommended method is, in almost all cases, trivial this is a big benefit at little cost.

I'm sure dawehner and tim.plunkett will have more to add on this subject.

An additional factor relating to make this change now is that if we wait until 8.1 the potential impact on contrib will be much bigger because by that time contrib modules will be relying on each others APIs being stable.

Obviously we need to get this information, and more, into the issue summary.

Thinking about it the, @deprecateds should not be "removed before 9.0.0," they should be "removed in 9.0.0 or later". I will update those comments in an updated patch. We can't remove them in 8.x.x without breaking lots of things.

Here is the corrected patch.

I think Xano's point is about implementation of interfaces. If your class implements EntityInterface but does not inherit Entity, you suddenly have an abstract class. Fortunately, if you have any automated tests this will fail testing so it is easy to spot, either in core or in contrib. Once found, this is trivial to fix.

Exactly. Automated tests will help, but that still means we're introducing a BC break right before 8.0.0. Even if that is accepted, we should document that in the issue summary here before this is RTBC'ed.

Thinking about it the, @deprecateds should not be "removed before 9.0.0," they should be "removed in 9.0.0 or later". I will update those comments in an updated patch. We can't remove them in 8.x.x without breaking lots of things.

The original form is correct, and is to be interpreted as in 9.0.x, before 9.0.0.

Updated the summary to try to improve it.

On the wording of the @deprecated tags, I guess that illustrates a cultural difference between the language of somebody working on core who sees 8.0.0 (and therefore 9.0.0) as being culmination of 5 years of work, and the language understood by somebody who normally works in site building and contrib (me) where 8.0.0 is the start of Drupal 8 and 9.0.0 is the start of Drupal 9.

How about "removed in 9.0.x" as something that will mean the same for both sets of Drupallers?

Changing "removed in 9.0.0" to "removed in 9.0.x"

That is also incorrect. 9.0.x will continue to exist until after 9.0.0 has come out.
Please leave the message the way it was originally. It literally means that we remove it before the first stable Drupal 9 release, but only after we have already started working on Drupal 9.

This is a change to an interface, which implies a BC break.

It does not imply a break, it IS a break. Let's be honest and clear about this, however mitigated the break may be.

Re-rolling the patch,keeping "removed in 9.0.0" unchanged.

@alexpott, @effulgentsia and I agreed on making this an RC target. This would need to go in today because of the entity interface change, so we should review and add a change record if possible.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -400,6 +409,13 @@ public function link($text = NULL, $rel = 'edit-form', array $options = []) {
   +    return parent::toLink($text, $rel, $options);
   

   What is the purpose of this method override? It does not seem to do anything in its current state.

2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -101,6 +101,25 @@ public function bundle();
   +   * Deprecated alias for toUrl().
   

   We already mark this deprecated using @deprecated, so we may want to keep the old description. Whatever we do, this must start with a verb.

3. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -128,11 +147,17 @@ public function label();
   +   * Deprecated way of getting the public URL for this entity.
   

   Should start with a verb. Maybe leave the old description intact here as well?

4. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -142,11 +167,17 @@ public function urlInfo($rel = 'canonical', array $options = array());
   +   *   fully set up, or it may throw an exception.
   

   What does fully set up mean, and why do we document that here and not on the method that this warning is about?

5. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -159,10 +190,36 @@ public function url($rel = 'canonical', $options = array());
   +   *   \Drupal\Core\Link instead of a string.
   

   Superfluous warning. The ::toLink() method already documents its own return value.

6. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -159,10 +190,36 @@ public function url($rel = 'canonical', $options = array());
   +   * Generates the HTML for a link to this entity.
   

   This method does not generate HTML.

I should note that the reason we allowed the BC break of adding methods to the interface was that it's an edgecase to implement EntityInterface directly rather than using one of the base classes -- the entity system assumes in a lot of places that stuff is either content or config, so you'd already be fighting an uphill battle. And this is a priority because of the issues with the API surface. We would not however allow this change after this point.

I'll work on drafting a change record.

Created https://www.drupal.org/node/2614344 for this issue and all the related issues. (I added the ones I found, though I believe there are others.)

#37

1. Nothing, I removed all three superfluous overrides.
2. Restored much doxygen, just added @deprecated
3. Same
4. It means it needs an id, documented such, adjusted relevant code
5. removed
6. By the time I got here it was gone from the fixes for 2.

Interdiff still against 34, sorry, one method too many nuked.

Those overrides were to change the default param value from canonical to edit-form. Because that's the important route for config entities, which don't have a canonical.
Please put it back?

With an inline comment maybe that says why? :) I'll accept test coverage for that as a followup since it's not a part of this issue's scope, but the fact that the patch passes demonstrates missing coverage.

This all looks clean and correct to me now and it has CR enough for this patch at least. Only a couple nitpicks, none of which need block the issue and all of which could be fixed on commit:

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
   @@ -387,6 +387,7 @@ public function urlInfo($rel = 'edit-form', array $options = []) {
   +    // Do not remove this override: the default value of $rel is different.
   
   @@ -394,12 +395,22 @@ public function url($rel = 'edit-form', $options = array()) {
   +    // Do not remove this override: the default value of $rel is different.
   ...
   +    // Unless language was already provided, avoid setting an explicit language.
   +    $options += ['language' => NULL];
   +    return parent::toUrl($rel, $options);
   

   Could be fixed on commit: we could add a similar comment there.

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -252,26 +259,33 @@ protected function linkTemplates() {
   +    // While self::toUrl() will throw an exception if the entity has no id,
   

   Nit: ID instead of id. Could be fixed on commit.

3. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
   @@ -101,7 +101,29 @@ public function bundle();
   +   * @see \Drupal\Core\Entity\EntityInterface::toUrl
   
   @@ -142,10 +168,37 @@ public function urlInfo($rel = 'canonical', array $options = array());
   +   * @see \Drupal\Core\Entity\EntityInterface::toUrl
   ...
   +   * @see \Drupal\Core\Entity\EntityInterface::toLink
   

   Could be fixed on commit: These need parens.

Of course, many usages remain in core of these methods. But at this point it is better to deprecate now and convert the usages later, since it is just a thin wrapper.

I'd consider this RTBC.

Crediting @xjm for the change record.

I pushed #45 as-is to 8.0.x. #49 and other docs improvements to this can be committed either before or after RC4 is tagged.

Thanks for working on this, and @xjm and @tim.plunkett for clarifying the changes!

I confirm the patch was committed with a proper commit message. However, the commit does not show up in this issue.

@Xano, yeah, there was some issue with cgit yesterday which might be related. It'll show up eventually I think.