Add a renderable object that is equivalent to l() [#1836730]

Meta issue #1843798: [meta] Refactor Render API to be OO
Follow-on from #1833920: [META] Markup Utility Functions

Here's a slightly altered sketch based on sun and Crell's posts in that issue. I'll post comments below this.

class Link extends Element {

  /**
   * Set up a bunch of default $options here.
   */
  var $html = FALSE;
  /**
   * @todo Copy current l() @param docs.
   */
  function __construct($title, $path) {
    // Save passed in arguments. They will only be processed when attributes are
    // accessed or when this element is rendered. Otherwise, nothing happens.
    $this->content = $title;
    $this->linkPath = $path;
  }
  
  // Have some methods for setting optional stuff.

  // From Element class:
  function __toString() {
    $this->ensureProcessed();
    return '<' . $this->tag . new Attribute($this->attributes) . '>' . $this->getContent() . '</' . $this->tag . '>';
  }
}

Comment	File	Size	Author
#9	twig_theme_system_rewrite.pdf	56.51 KB	Fabianx
#3	drupal-Link-1836730-3.patch	7.64 KB	steveoliver
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 9 November 2012 at 20:05

sun said this:

#31 looks interesting, but I'm concerned that a full-blown object for every single link, along with requiring multiple method calls to set up a simple link, will be detrimental in terms of performance.

A lot of l() calls only have text and path, so if we take those in the constructor, and have methods for the various $options, it might not actually be all that many extra method calls. The critical ones will be things like entity titles and menu links since there's usually dozens (if not hundreds) of those on page.

Comment #2

steveoliver CreditAttribution: steveoliver commented 11 November 2012 at 00:19

Could we be thinking of render objects as the evolution of render arrays?

Comment #3

steveoliver CreditAttribution: steveoliver commented 12 November 2012 at 18:34

Issue tags:

+link functions and templates, +renderable objects

File	Size
drupal-Link-1836730-3.patch	7.64 KB

OK, here's a working example, including two sample implementations:
1. The "Add new content" link (on the home page when no nodes exist)
2. The toolbar toggle link (up/down arrow on right side of toolbar).

Comment #4

steveoliver CreditAttribution: steveoliver commented 12 November 2012 at 19:14

Also, in my quick search on object v. array performance, the first hit I got was "PHP objects vs arrays performance myth (a simple benchmark)". #fwiw.

Comment #5

jenlampton

she / her

CreditAttribution: jenlampton commented 12 November 2012 at 19:21

Just for the record, I think this is a horrible, horrible idea.

Most current Drupal developers are not familiar with OOP, and they shouldn't need to be in order to write a simple module. Turning a handy little utility like l() into a mess like this is going to make Drupal very, very hard to learn.

The structure we are seeking here should come at a much higher level than l() itself. Let's take pagers for example: the pager can provide structure that l() can use, and something like theme_pager or pager.html.twig can use the structure provided in the pager object/array and then call l() - the simple little utility function, as it is now.

Comment #6

steveoliver CreditAttribution: steveoliver commented 12 November 2012 at 19:27

Also for the record, I don't think l() should be refactored or replaced. I think any such renderable object should be part of a separate set of tools. I think l() should remain the simple utility function that it is, with one of the only changes being -- simplification -- #1778610: Remove the check for a link template from l(), have l() always output just a string.

Comment #7

jenlampton

she / her

CreditAttribution: jenlampton commented 12 November 2012 at 19:42

Title:

Refactor l() to a renderable object

» Add a renderable object that is equivalent to l()

I get the intent, but I still completely disagree with the solution - or even believe that one is necessary.

Since we are not using theme_link anywhere now, and it was only added "just because" we thought all markup needed to go through the theme layer (which we now believe to be a false assumption) removing it does no harm, and we don't need to add anything else - especially something this abstract.

Yes, links need structure to be "renderable". But I still hold that the structure we need should be added at higher levels. This is made obvious because we never need to theme/override every link. Do we need to override pager links? yes. that's why we have theme_pager() and it can contain the structure we need to override the links within that pager. Do we need to override menu links? yes. that's why we have theme_menu(). But we absolutely do not need a theme_link.

This is Drupal's classic problem of over thinking everything and making things way more complicated than necessary, and I'm against it.

Comment #8

Crell CreditAttribution: Crell commented 12 November 2012 at 19:50

Jen: So are you arguing for "no, you don't get to retheme a elements, sry"? (I am OK with that if you are, but I'm not sure.)

In general I agree with doing things at a higher-level where possible. However, I do want to call out this point:

Most current Drupal developers are not familiar with OOP, and they shouldn't need to be in order to write a simple module.

That ship has already sailed, and you'll need to know at least some OOP to write anything meaningful in Drupal 8. So it goes.

Comment #9

Fabianx CreditAttribution: Fabianx commented 12 November 2012 at 21:50

File	Size
twig_theme_system_rewrite.pdf	56.51 KB

We reached some consensus on IRC (#drupal-twig) and talked a lot more.

I attach an unfinished draft of some overview of how theme(), render(), etc. really work to help with that work.

Comment #10

c4rl CreditAttribution: c4rl commented 13 November 2012 at 01:09

Title:

Add a renderable object that is equivalent to l()

» Have l() be a wrapper for a renderable object

To clarify #7, I still believe that theme_link may be necessary, as denoted in #1833920: [META] Markup Utility Functions here https://drupal.org/node/1833920#comment-6707080

I don't think the API has to change, or the DX. I am not seeing this as a top-level call, but simply something that l() would wrap.

That is, developers would never use the Link class directly, they would still use l(). Is this correct?

I am thinking that the Element() interface may replace theme_html_tag() and be an answer to where I was going with Markup Utility Functions. I need to give this some more thought, but I think this is a worthwhile discussion at the moment.

Comment #11

steveoliver CreditAttribution: steveoliver commented 13 November 2012 at 18:08

I think using existing functions as wrappers for "smarter" objects makes enough sense to me.

This way, we get:

No unnecessary processing like url(), Attribute(), etc. for variables that aren't printed (thanks, __toString()).
Variables that are drillable all the way down through the theme layer, without needing to run each element through theme()
No new API.

Comment #12

Fabianx CreditAttribution: Fabianx commented 13 November 2012 at 18:56

Title:

Have l() be a wrapper for a renderable object

» Add a renderable object that is equivalent to l()

That is very hard to do without any additional things technically ;).

Let me tell you some tales from having return l(), t(), etc. all Twig_Markup objects within core :) ...

It stil is a good idea, but l() should really return plain text, and our new component should do the lifting of creating render arrays or other lazy objects ...

The problem is not solved as often times you find:

'<div>' . l(...) . '</div>';

Or something to that effect, which renders l() correctly, but defeats the purpose ...

I have a better solution using only render arrays :) and will write it up tomorrow or later today ...

Comment #13

jenlampton

she / her

CreditAttribution: jenlampton commented 16 November 2012 at 03:02

@Crell, yeah, that's exactly what I'm saying. If you want to change the link at the template level, then just write your own anchor tag in the template. If you want to change it at the module level, you'll have to alter the renderable object/array that the link is inside: $pager['link']['attributes']['title'] = 'changed me!'

I think we came to a consensus about how this could be done this week, and I'm excited to see the outcome :)