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

This is similar to #1213510: A modern t() and #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess().

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() {
    return '<' . $this->tag . new Attribute($this->attributes) . '>' . $this->getContent() . '</' . $this->tag . '>';

Members fund testing for the Drupal project. Drupal Association Learn more


catch’s picture

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.

steveoliver’s picture

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

steveoliver’s picture

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).

steveoliver’s picture

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.

jenlampton’s picture

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.

steveoliver’s picture

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.

jenlampton’s picture

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.

Crell’s picture

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.

Fabianx’s picture

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.

c4rl’s picture

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

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.

steveoliver’s picture

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

This way, we get:

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

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 ...

jenlampton’s picture

@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 :)

jenlampton’s picture

jenlampton’s picture

Issue summary: View changes

Added link to renderable objects meta issue