Problem/Motivation

Currently template_preprocess_file_link() returns a link generated with Link::fromTextAndUrl()->toString(). This html can no be preprocessed by other themes/modules.

Proposed resolution

Return a renderable array in template_preprocess_file_link(), so other themes can add classes or update the title.

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit

User interface changes

None.

API changes

None.

Data model changes

template_preprocess_file_link() returns a renderable array for the link instead of html.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geoffray created an issue. See original summary.

geoffray’s picture

geoffray’s picture

Status: Active » Needs review

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

stefan.korn’s picture

This issue should surely be addressed soon because Drupal::l() is deprecated.

as per deprecated notice the replacement for Drupal::l() would be:

Link::fromTextAndUrl($link_text, Url::fromUri($url, $options));

But i would suppose the patch provided by @geoffray would also suit the deprecation notice.

Olarin’s picture

Priority: Normal » Major

This is a small, logical, and straightforward change, which shouldn't break anything for anybody (except possibly implementations of the file_link preprocessor that are specifically expecting to see the GeneratedLink object, and I can't really imagine what they could be usefully accomplishing with it), and is very useful to anyone who needs to modify that link for some reason. The attached patch worked well for me on a Drupal 8.6 installation.

Bumping to Major because of the deprecation issue, since D9 is theoretically less than a year away; apologies if that's inappropriate (the priority documentation doesn't really mention deprecation issues), but this has been sitting back-burner for a while and it seems like an easy win to me.

Olarin’s picture

Re-rolling against 8.8. Seems somebody already addressed the deprecation issue by using Link::fromTextAndUrl, but I believe it would still be more appropriate to return a render array here.

stefan.korn’s picture

Yes, it seems this has been addressed very recently with this issue: https://www.drupal.org/node/3047801

So maybe this issue could be closed as duplicate.

idebr’s picture

Issue summary: View changes
Status: Needs review » Needs work

#10 The problem described in the issue summary still applies: the generated link cannot be easily preprocessed or altered by themes.

+++ b/core/modules/file/file.module
@@ -1506,7 +1506,11 @@ function template_preprocess_file_link(&$variables) {
-  $variables['link'] = Link::fromTextAndUrl($link_text, Url::fromUri($url, $options))->toString();

Let's go with toRenderable() instead of toString() for the smallest possible change.

stefan.korn’s picture

Yes, the problem still applies. I've overlooked the "toString()" call they put in with https://www.drupal.org/node/3047801.

Here comes the patch with toRenderable() which will result exactly in what is provided by the previous two patches.

But it seems even not to be necessary to use toRenderable() at all here, since Twig can handle the Link object too.

stefan.korn’s picture

Status: Needs work » Needs review
idebr’s picture

Status: Needs review » Reviewed & tested by the community

#12 Looks good to me. Typically Drupal core returns renderable arrays instead of class objects, so let's go with toRenderable() in line with the issue summary.

larowlan’s picture

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6bae96a and pushed to 8.8.x. Thanks!

Added and published a change record https://www.drupal.org/node/3074716

  • larowlan committed 6bae96a on 8.8.x
    Issue #2913487 by geoffray, stefan.korn, Olarin, idebr: Return a...

Status: Fixed » Closed (fixed)

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