Problem/Motivation

* #type elements can be integrated with #theme but in the usual case we want to render them fast internally (e.g. links).
* #type elements add #pre_render => '...' in hook_element_info().
* The #pre_render type callback like (drupal_pre_render_link) just setup #markup and render the link already.

This is problematic for "drillability" and "lazy rendering".

* In many cases the render system is "abused" by setting #pre_render calls, because very often actual HTML is rendered here already and what is called a pre_render in reality is a render function.

Proposed resolution

We find several properties:

* #theme is just a specific form of rendering the same input than #type can do

Therefore #type is like a base type (base class), where #theme is a specific implementation of it.

* There is no reason that actual rendering could not be done after the #pre_render phase.

For now this would look like this (pseudo-code):

element_info()
call(#pre_render)

$theme_is_implemented = FALSE;

if (#theme) {
  call(#theme())
  $theme_is_implemented = !empty(#children);
}

if (!$theme_is_implemented && !empty(#render_function)) {
  call(#render_function)  
  drupal_render_children()
}

The bigger picture of how this enables the theme layer to work in combination with drupal_render is described in @todo, but the short version is that theme_info() just returns #render_function and #theme_info properties (which are merged) and such can override the default rendering of a #type with its own methods.

Remaining tasks

* Create patch and convert #type link as an example (drupal_pre_render_link -> drupal_render_link)
* Tests, Docs, Profiling
* Get patch commited :-)

* Create list of all #type element_info that need conversion to new API and split into sub issues.

The hitlist

This is the list of pre_render functions - https://api.drupal.org/api/drupal/8/search/pre_render

From comment #5 we have:

pre_render should not modify the variables to aid in rendering

So, here's the list of all pre_render functions that break that rule - we consider these pre_render functions an abuse of the Render API:

- @todo

Here's a list of pre_render functions that simply restructure an element, or provide structure (such as for the FAPI) without modifying/processing/sanitizing values as required for rendering markup, and are considered OK:

- @todo

User interface changes

* None.

API changes

* Introduce #render_function property for drupal_render
* Discourage usage of #pre_render to render something into #markup or do other "render" tasks.

@todo

Files: 
CommentFileSizeAuthor
#7 2052253-7.patch5.53 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2052253-7.patch. Unable to apply patch. See the log in the details link for more information. View
#3 2052253-3.patch5.58 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,953 pass(es). View
#3 2052253-2-3-interdiff.txt721 bytesthedavidmeister
#2 2052253-2.patch5.52 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 57,881 pass(es). View

Comments

Fabianx’s picture

#2044105: Introduce #alters for drupal_render() is an implementation of the #render_functions for the #render_part.

thedavidmeister said it should probably wrap #children (like #them_wrappers later) and if we continue to special case #theme, I agree now.

thedavidmeister’s picture

Title: [META] Add #render_function property to drupal_render() and convert #type "#pre_render -> #markup" calls to use it » [META] Add #render property to drupal_render() and convert #type "#pre_render -> #markup" calls to use it
Status: Active » Needs review
FileSize
5.52 KB
PASSED: [[SimpleTest]]: [MySQL] 57,881 pass(es). View

Here's a patch that contains the relevant half of what was posted at the issue linked at #1.

thedavidmeister’s picture

FileSize
721 bytes
5.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,953 pass(es). View

Fabianx asked in IRC for is_callable() and call_user_func() to be used. Patch attached.

thedavidmeister’s picture

incidentally, I think the "and convert #type "#pre_render -> #markup" calls to use it" part of this issue should be sub/followup issues and not in the first patch.

Fabianx’s picture

A relevant architecture chat:

Fabianx: I think of #type and #render as _fast_ default render functions.
[3:16pm] Fabianx: If you want flexibility go via the theme system.
// ...
[3:24pm] Fabianx: thedavidmeister: still we need to make something that makes sense.
[3:24pm] Fabianx: And originally #type was "rendered" via the theme system directly.
[3:25pm] Fabianx: Then #theme was introduced for FAPI and all was good.
[3:25pm] Fabianx: Then render arrays came and #type were using #pre_render to not have to go via the theme system ...
[3:26pm] Fabianx: However, theoretically (even if we "should" not do that): I could use a
[3:26pm] Fabianx: #theme => link__mysuggestion
[3:26pm] Fabianx: directly.
[3:26pm] Fabianx: without using #type.
[3:26pm] thedavidmeister: yeah
[3:26pm] Fabianx: And as long as suggestion handling is in theme().
[3:26pm] Fabianx: Having #pre_render do any kind of needed processing is either:
[3:27pm] Fabianx: * doing double work (like it now would be)#
[3:27pm] thedavidmeister: no, you're right
[3:27pm] thedavidmeister: i get it
[3:27pm] Fabianx: * not doing enough work, i.e. #theme => X cannot work without #type => Y.
[3:28pm] thedavidmeister: i wanted #type to be required
[3:28pm] thedavidmeister: but that ship sailed
// [...]
thedavidmeister: basically though
[3:29pm] thedavidmeister: that means pre_render is useless if you have render
[3:30pm] thedavidmeister: and the point of render is to "inline everything"
[3:31pm] thedavidmeister: Fabianx: I don't really mind, but we should maybe make that clearer in the issue
[3:31pm] thedavidmeister: Fabianx: that the goal is to not just move away from setting #markup in #pre_render but to move away from setting anything to anything in #pre_render
[3:32pm] thedavidmeister: Fabianx: except to maybe do a little re-shuffling of the element structure before it is rendered
[3:32pm] Fabianx: thedavidmeister: yes
[3:32pm] thedavidmeister: Fabianx: as in.. "pre_render should not modify the variables to aid in rendering"
[3:32pm] thedavidmeister: Fabianx: confusing, but… that's essentially it
[3:32pm] Fabianx: thedavidmeister: yes, agree.
[3:33pm]
thedavidmeister’s picture

Ah, the bit Fabianx missed from the chat log is the new sub-issues we wanted to open:

- Create a proof-of-concept patch demonstrating how #type 'link' would be converted to the #render instead of #pre_render -> #markup
- Create a list of every current usage of #pre_render that we consider "abusive" so we know what needs to be cleaned up and if we need to do anything further to drupal_render() to accommodate the cleanup

thedavidmeister’s picture

FileSize
5.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2052253-7.patch. Unable to apply patch. See the log in the details link for more information. View

I removed is_callable() after discussing with Fabianx in IRC - nothing else in drupal_render() does this check.

Patch needed a re-roll.

I cleaned up the docs a little.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

Not sure how to handle functions like ajax_pre_render_element()

The processing done here is not specific to a particular #theme or even #type, it's just specific to the whole FAPI in general, which makes it very difficult to compare directly to any processing that happens within theme(), but is doing things that would otherwise be considered "processing for rendering" - in the form of creating URLs from paths.

It strikes me that most of the discussion I've had so far in IRC and the issue queue has been in the context of pre_render/preprocessing/altering that are designed to work for only a specific #type or #theme. Re-usable #types and processing is what we're trying to encourage more of, not narrowing the scope of each processing function to be specific to a single element.

I'm just thinking aloud here, but would it be beneficial to support multiple #type's per element someday? merging in default attributes successively.

Instead of using #type 'checkbox' with #ajax => TRUE/FALSE which forces all FAPI elements to hit ajax_pre_render_element() just to be skipped over every time there is no AJAX for an element (slow), why not allow #type => array('checkbox', 'ajax') and let the ajax pre_render function be merged into the array of pre_renders provided for 'checkbox' if required?

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

Fabianx’s picture

Tagging ...

Fabianx’s picture

Issue summary: View changes

Updated issue summary.

markcarver’s picture

7: 2052253-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: 2052253-7.patch, failed testing.

Cottser’s picture

Bump, are we still interested in this? It's filed as a major task but hasn't seen any activity in quite some time.

sun’s picture

catch’s picture

Status: Needs work » Closed (duplicate)