Problem/Motivation

Drupal 8 shipped with a bunch of functions that were immediately marked as deprecated. I can see the use for some leftovers such as drupal_render() and entity_load_multiple() but I wonder why there were new methods introduced that are also deprecated already.

For instance, if you actively avoid using deprecated functions, you have to write this to generate an entity link:
\Drupal::service('renderer')->render($entity->toLink()->toRenderable()); instead of $entity->link();

Is this really the direction we want to go in? Why can't we keep the link() method as a shorthand for the rendered Link object for example? I'm worried that we will scare off less experienced developers this way or have completely illegible code as a result of having to be so verbose.

Alternatively: is it okay to use deprecated functions to avoid the above verbosity or do you risk having your code broken somewhere throughout the D8 cycle?

Proposed resolution

  • Un-deprecate toString() and explain the situations in which it may be used.
  • The following is the non-deprecated way of doing it then: $entity->toLink()->toString()

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#20 drupal-undeprecate_link_to_string-2628754-20.patch1.32 KBkristiaanvandeneynde
#10 link_tostring_should-2628754-10.patch855 bytescilefen
#10 interdiff.txt923 bytescilefen
#8 2628754-8.patch785 bytesdawehner

Comments

kristiaanvandeneynde created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented 
 \Drupal::service('renderer')->render($entity->toLink()->toRenderable());

If you really need to render it to a link, which ideally you would not have to, you could also just do: $entity->toLink()->toString()

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I wanted to do that, but toString() is deprecated as well :D

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I don't understand why toString() is deprecated, but at that point, it's a link/url system problem, not in entity system :)

bojanz’s picture

bojanz commented

toString() being deprecated makes no sense, since there's no alternative.

dawehner’s picture

dawehner
Primary language German
commented

Right, it should be just decouraged to be used, as well, on render API level you should just use render arrays.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I know I can and should just use render arrays. I'm highlighting this issue as it's a clean example of how complex some simple tasks have become. If someone asks you to return a link to an entity as a string -regardless the reason- the code you have to write is all but pretty.

Also, I can't tag this as "link system" since it doesn't exist and link.module isn't correct here.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2628754-8.patch785 bytes

So what about the following statement?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: DX: Reduce code needed to do simple things? » Link::toString() should not be deprecated
Component: entity system » routing system
Category: Support request » Bug report
Issue tags: +Needs issue summary update

Works for me. Maybe a native speaker can review and improve the wording?

Updating component and issue title. Also needs an issue summary update.

cilefen’s picture

cilefen commented
StatusFileSize
new interdiff.txt923 bytes
new link_tostring_should-2628754-10.patch855 bytes

I did the native-speaker check. I also moved it to the full function description because appearing after the @return annotation (because it replaced a @deprecated), it appears wrongly to be part of the return documentation.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Thanks, just needs an issue summary update now I think. I'd keep the current text in problem description and add a proposed solution for un-deprecating toString().

cilefen’s picture

cilefen commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue summary: View changes
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Change makes sense to me. We shouldn't deprecated methods that have no real replacement/acceptable alternative way of doing things.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Agreed, I thought I was losing my mind here :) Generating textual links should not be such a pain.

dawehner’s picture

dawehner
Primary language German
commented

It would be nice if we have a semantic tag for, this is not wrong, but its suggested to do something else, but for now, there is nothing else.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Link.php
@@ -140,11 +140,13 @@ public function setUrl(Url $url) {
+   * It is discouraged to use toString() in any code which renders something in

Don't we know what that something is? i.e a Link object. Also I think the paragraph could be more clearly written as...

Do not use this method to render a link in an HTML context. In an HTML context, self::toRenderable() should be used so that render cache information is maintained. However, there might be use cases such as tests and non-HTML contexts, where calling this method directly makes sense.

Also an @see to ::toRenderable() should be added.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -Needs issue summary update
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-undeprecate_link_to_string-2628754-20.patch1.32 KB

Done #18

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs review » Reviewed & tested by the community

All points were addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I removed a superfluous comma I added to the text.

Committed 5bc305c and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 9993a87 on 8.1.x 
    Issue #2628754 by cilefen, kristiaanvandeneynde, dawehner, Berdir: Link...

  • alexpott committed 5bc305c on 
    Issue #2628754 by cilefen, kristiaanvandeneynde, dawehner, Berdir: Link...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

SlayJay’s picture

SlayJay commented

Is there any place setup to discuss the bigger issue?

\Drupal::service('renderer')->render($entity->toLink()->toRenderable());
vs.
$entity->toLink()->toString()

is happening all over the place in core, and I really thing this is a disastrous direction to head in.