Theme preprocess functions have to futz with $page_object = $variables['page']['#page'] in weird ways [#2218271]

Problem

Theme preprocess functions have to futz with HtmlFragment/HtmlPage objects in extremely weird ways:

function template_preprocess_install_page(&$variables) {
  $page_object = $variables['page']['#page'];
  $attributes = $page_object->getBodyAttributes();
  $classes = $attributes['class'];

  $classes[] = 'install-page';

  $attributes['class'] = $classes;

That is the result of some glue code, which was meant to be temporary only.
Themers will not be able to grasp that.

Tagging in all appropriate forms, because resolving this is a major API change that needs to happen before beta, and I only coincidentally stumbled over this problem space in some unrelated work. — I don't know whether there is a critical issue about this already.

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 17 March 2014 at 12:15

The same 'glue code' came up in #2215719: Cache tags set by #pre_render callbacks are lost in page cache (which is critical but won't fix the theming issue), where Wim Leers also noticed that exactly the same thing is done in two places at the moment. I don't know of any other open issues that are tackling this, the existing code is very confusing and this is an absolutely central part of the system.

https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Page!DefaultHtmlF...

https://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/t...

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 17 March 2014 at 13:11

There are two possible options in which we could resolve this — I don't know what the original plan or idea was, so just throwing them out there:

Make _theme() pass the HtmlFragment/HtmlPage object as a separate + first argument:

function template_preprocess_install_page(HtmlFragmentInterface $fragment, array &$variables) {
  $attributes = $fragment->getBodyAttributes();
  $classes = $attributes['class'];
  $classes[] = 'install-page';
  $attributes['class'] = $classes;

Note: HtmlPage extends from HtmlFragment.

Note: I do not expect themers to know, understand, and use PHP type-hints, so that's just for clarity here.

Extend the Attribute with the most common jQuery-alike methods:

function template_preprocess_install_page(HtmlFragmentInterface $fragment, array &$variables) {
  $fragment->getBodyAttributes()
    ->removeClass('odd even')
    ->set('id', 'main')
    ->addClass('install-page');

Comment #3

catch

he/him

English

CreditAttribution: catch commented 17 March 2014 at 13:32

Category:

Bug report

» Task

Moving to task, this is an API issue, not a functional bug.

Comment #4

sun

German

Karlsruhe

CreditAttribution: sun commented 17 March 2014 at 13:42

Oh. An alternative option would be:

Make HtmlFragment wrap $variables via ArrayObject:

function template_preprocess_install_page(HtmlFragmentInterface $variables) {
  $variables->getBodyAttributes()
    ->removeClass('odd even')
    ->set('id', 'main')
    ->addClass('install-page');

  $variables['hocus'] = 'pocus';

A bit dirty, but wouldn't require us to change all theme preprocess functions and theme functions throughout core at once.

Comment #5

jessebeach CreditAttribution: jessebeach commented 24 March 2014 at 09:31

I'm really torn about this issue. First, because I desperately want to clean up this part of Drupal. And I really like the proposal to provide a jQuery-like API to manipulate HTML Fragments. This is a incredibly clever suggestion that makes scary PHP seem more approachable to themers.

On the other hand, this issue proposes an enormous change very close to beta. We need to discuss whether this issue, although desirable, is completely necessary and thus blocking beta.

Given the suggestion in #3, this change could be introduced as a beta target, allowing us to roll a beta without this being a blocking issue.

I propose we downgrade this issue to beta target.

Comment #6

dawehner

German

CreditAttribution: dawehner commented 24 March 2014 at 10:35

While it is certainly nice to have the page object exposed and some nice clean API on the theming level, I wonder whether this is a good idea
to begin with, given how most of the theming code works at the moment. A simple alternative could be to just pass along the variables when building the render API.

Comment #7

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 24 March 2014 at 21:09

Isn't directly related, but #2090967: Extend page.html.twig with html.html.twig changes code related to this, thought I'd cross post.

Comment #8

catch

he/him

English

CreditAttribution: catch commented 25 March 2014 at 15:09

Issue tags:

-Release blocker

Downgrading this to beta target - I'd allow an API change that improves this to go in after beta. It'll be an addition if we add any new capabilities, and the only actual API change would be in template_preprocess_page().

Also removing 'release blocker' tag, this is critical already.