[D8][DX] Form API: Backport D8 `html_tag` element changes

klonos commented 5 years ago

theme_html_tag() was renamed to theme_head_tag() and repurposed in the early days of Backdrop (see #452 and the respective change record).

The function theme_html_tag() has been renamed theme_head_tag(), and as such, should only be used for adding HTML tags within the <head> tag on the page.

Our brethren in Drupal 7 are also trying to get rid of that theme function, but instead of simply renaming/deprecating it, they are trying to backport the '#type' => 'html_tag' functionality from Drupal 8: https://www.drupal.org/project/drupal/issues/2981726 (draft change record).

The intention is to be able to do something like this:

  $form['some_html_element'] = array(
    '#type' => 'html_tag',
    '#tag' => 'label',
    '#value' => t('Some awesome label goes here'),
    '#attributes' => array(
      'id' => 'my-label',
      'for' => 'some-input'
      'data-attribute' => t('Some data-* attribute'),
    ),
  );

...instead of having to resort to something like this:

  $form['some_html_element'] = array(
    '#type' => 'markup',
    '#prefix' => '<label id="my-label" for="some-input" data-attribute="Some data-* attribute">',
    '#markup' => t('Some awesome label goes here'),
    '#suffix' => '</label>',
  );

Benefits:

the HTML tag can easily be overridden with something like $form['some_html_element']['#tag'] = 'p';, instead of having to override $form['some_html_element']['#prefix'] and $form['some_html_element']['#suffix'].
you can easily override individual attributes, as opposed to using '#prefix' which "hardcodes" the attributes, and forces you to parse and re-write the entire $form['some_html_element']['#prefix'].
it'd be backwards-compatible with what is (will be) in D7, or at least will require less effort to convert things to Backdrop (change '#theme' => 'html_tag', to '#type' => 'html_tag',)
will allow nested elements, once support for 'child' => array() has been backported.

Original report

This is the respective issue for https://www.drupal.org/project/drupal/issues/2981726 in the D7 core queue:

One year ago, the Drupal 8 html_tag has been updated and it's now able to render nested tags, see the CR here #2887146.

I (@drupol) backported the functionality to Drupal 7 so now, we can render this render array properly, including the children.

To test the functionality, run this code before and after applying the patch in /devel/php:

$render_array = array(
  '#type' => 'html_tag',
  '#tag' => 'h1',
  '#attributes' => array('class' => 'title'),
  'children' => array(
    array(
      '#type' => 'link',
      '#title' => 'Link title',
      '#href' => '/',
      '#attributes' => array('class' => 'inner'),
    ),
    array(
      '#theme' => 'link',
      '#text' => 'Link title',
      '#path' => '/',
      '#options' => array(
        'attributes' => array(),
        'html' => FALSE,
      )
    ),
  )
);

$html = render($render_array);

debug($html);

Without the patch:
<h1 class="title" />
With the patch:

<h1 class="title">
  <a href="/" class="inner">Link title</a>
  <a href="/">Link title</a>
</h1>`

Another example:

$render_array = array(
  '#type' => 'html_tag',
  '#tag' => 'h1',
  '#attributes' => array('class' => 'title'),
  '#value' => 'value',
  'children' => array(
    array(
      '#type' => 'link',
      '#title' => 'Link title',
      '#href' => '/',
      '#attributes' => array('class' => 'inner'),
    ),
    array(
      '#theme' => 'link',
      '#text' => 'Link title',
      '#path' => '/',
      '#options' => array(
        'attributes' => array(),
        'html' => FALSE,
      )
    ),
  )
);

$html = render($render_array);

debug($html);

Without the patch:
<h1 class="title">value</h1>`
With the patch:

<h1 class="title">
  value
  <a href="/" class="inner">Link title</a>
  <a href="/">Link title</a>
</h1>

Change record

Proposed change record: https://www.drupal.org/node/2982025

herbdool commented 5 years ago

This might help explain why we'd want to make this change (which is BC) https://www.drupal.org/project/drupal/issues/2694535:

Problem/Motivation

Drupal has no easy way to dynamically create SVGs, currently you either have to write the markup yourself and use string concatenation or use the "inline_template" render element.

Original summary:

I am a contributed module maintainer and am working on a port of a module that dynamically generates inline SVG in page content. It does so to produce charts, graphs, etc based on SQL data. Currently the rendering system removes all inline SVG from markup. Most of the suggestions I've seen suggest writing custom theme layers to embed the SVG, but in my case, I don't know how many SVG graphs will be in a document in advance.

laryn commented 3 years ago

I'm porting OG, which (at a glance) appears to use html_tag in a few places, so I'd support adding this.

Update: maybe I spoke too soon. Digging deeper to see if this is related or not...

drupol commented 3 years ago

I remember being very involved into this thing ! Curious to see the outcome :-)

klonos commented 2 years ago

Hey @jenlampton I saw that you've reacted with 👎🏼 to this feature request (in the duplicate #5223). Curious to know why.

I'm coming across some (many?) 7.x contrib that are using some form of return theme('html_tag', array( ... )); which could use this. Also having D8/9 feature parity is good, right?

PS: I think that this would also help with svg support (programatically rendering them etc.).

klonos commented 2 years ago

The last comment currently in the D7 issue to backport this is from 3years ago. Among other things, it says (emphasis mine):

...

7.x is nearing EOL. If new features (that aren't present in future versions) are needed, they should be kept in contrib. If it's because there's resistance to the way 8.x works, perhaps the sites should instead migrate to Backdrop instead of trying to turn 7.x into something it was never meant to be in the first place. ... ...

Well if we don't have this feature in Backdrop either, then what's the point? Right?

klonos commented 2 years ago

I also just re-read through Remove theme_html_tag() from the theme system and I'd be fine with this:

Problem/Motivation

html_tag does not need to be theme function. It's bad for performance and not a good use case for alterable output via the theme system.

Proposed resolution

Convert theme_html_tag to a pre-render callback and remove the theme function.

...but so long as we support '#type' => 'html_tag' in the Form API. In other words:

API changes

Any calls to theme('html_tag'); or within a render array '#theme' => 'html_tag' need to be converted to '#type' => 'html_tag'. Drupal 8 core does not contain either of these and always uses '#type' => 'html_tag', but Drupal 7 core has calls to theme('html_tag'); and contributed modules probably contain both.

We've already sort of made the API change of "removing" the theme function. All I'm saying is that we should at least support '#type' => 'html_tag' the same way that D8/9 support it. It will be the "upgrade path" for 7.x contrib that moves to Backdrop.

indigoxela commented 2 years ago

With "programmatic svg rendering" this topic definitely caught my attention. :smile: ("D8 parity" usually makes me yawn...)

jenlampton commented 2 years ago

Hey @jenlampton I saw that you've reacted with 👎🏼 to this feature request (in the duplicate #5223). Curious to know why.

It goes against 3 of our principals: Simplicity, Speed + Performance, Focus

Simplicity: One of the biggest problems with Drupal 7 theme system complexity is that there are so many theme functions for so many things, and the size of the elements with theme-able output are getting smaller and smaller. This makes it very hard for front-end developers to figure out where markup is coming from, and where to override it when necessary. In early versions of Drupal we had overridable output for big things like nodes, comments, categories. Overriding items at this level makes sense to people new to the platform.

The introduction of render arrays in D7 was one of the leading causes of overridable items getting smaller and smaller, with this theme function being the worst example. We don't need a dedicated function to render something both as simple and as generic as a "HTML tag". If we do need output for a single specific HTML tag, like an input, for example, that can/should have its own function for output -- and it does :)

Performance: In addition to making things hard to understand, having a dedicated function to render individual HTML tags is bad for performance.

We've already sort of made the API change of "removing" the theme function. All I'm saying is that we should at least support '#type' => 'html_tag' the same way that D8/9 support it.

Focus : Why? We should not pursue any changes to core without a valid use-case. We shouldn't do anything "just because Drupal does it" unless we have our own good reasons that stands on their own merit. An issue like this that proposes a change to core without anyone actually needing the change makes me cringe a little -- hence the 👎

If/when we come across an issue that can't be done without this change, we can re-evaluate.

klonos commented 2 years ago

So @jenlampton just to clarify, you are against both theme_html_tag()/'#theme' => 'html_tag' and the '#type' => 'html_tag' FAPI addition. Right? I understand the reasons for the former, but not the latter.

Why? We should not pursue any changes to core without a valid use-case. ...

It's not just a matter of "just because Drupal does it" and backwards compatibility and/or feature parity, it's also about doing things properly. For instance, I'm sure that something like this would get the job done while also "simplifying" things and eliminating the need for theme functions etc.:

$legend = '<span class="fieldset-legend">' . t('Everything is markup FTW!!!') . '</span>';
$form['everything_is_markup_after_all'] = array(
  '#type' => 'markup',
  '#prefix' => '<fieldset class="form-wrapper"><legend>' . $legend . '</legend>',
  '#markup' => '<div class="fieldset-wrapper">' . t('Adding all my contentz HTML stylez yo.') . '</div>',
  '#suffix' => '</fieldset>',
);

...but we're using '#type' => 'fieldset' instead.

jenlampton commented 2 years ago

you are against both theme_html_tag()/'#theme' => 'html_tag' and the '#type' => 'html_tag' FAPI addition. Right?

yes.

I understand the reasons for the former, but not the latter.

It doesn't make sense to add a single #type that works differently than everything else in core. What do you have against the current '#type' => 'head_tag'?

It's already a very small change when upgrading from Drupal 7: change '#theme' => 'html_tag', to '#type' => 'head_tag'.

This would work for sites that use '#theme' => 'html_tag' in some way that't not possible to handle with other solutions -- which, again, is a use-case we don't even know exists.

...but we're using '#type' => 'fieldset' instead.

Right, because we have a use-case for fieldsets. :)

We still don't have a use-case for a generic HTML tag that can't also be solved in a more "proper" way, like theme_input() or '#type' => 'input'.

klonos commented 2 years ago

OK, so here's my understanding of this:

we've removed theme_html_tag() in Backdrop, which breaks backwards compatibility (against our principles)
we've introduced theme_head_tag(), which does exactly the same thing as theme_html_tag()

The above is confusing for developers, and if it were me, I'd still "abuse" theme_head_tag() to work my way around porting modules, which would defeat the purpose, and get us back to the reasons we removed theme_html_tag() for (performance etc.).

What I believe we need to do is:

restore theme_html_tag(), but only as a wrapper function for theme_head_tag(), with a watchdog_deprecated_function() that points people to the change record.
update the documentation for theme_head_tag() to clearly state that the intended purpose is to be adding HTML to the <head> of pages (#5452) and perhaps also add that this will be "enforced" in the future(?).
slate theme_html_tag() for complete removal in Backdrop 2.x
backport drupal_pre_render_html_tag() as backdrop_pre_render_html_tag(), and add drupal_pre_render_html_tag() in /core/includes/drupal.inc if it also gets added to D7 (with a watchdog_deprecated_function() etc.).
make sure that '#type' => 'html_tag' in FAPI happens via backdrop_pre_render_html_tag() instead of theme_html_tag(), so that we can get rid of '#theme' => 'html_tag' and theme('html_tag');.

Perhaps I'm not understanding something, or missing a key point. Perhaps discussing this over a dev meeting or sprint session in person would help the situation.

herbdool commented 2 years ago

There's nothing stopping contrib modules from just using the changed module name. Easy peasy. I don't see the need for a wrapper since we've got lots of places where contrib module maintainers need to update code. And this one is rather easy.

jenlampton commented 2 years ago

I agree.

On Thu, Jan 13, 2022, 4:30 PM Herb @.***> wrote:

There's nothing stopping contrib modules from just using the changed module name. Easy peasy. I don't see the need for a wrapper since we've got lots of places where contrib module maintainers need to update code. And this one is rather easy.

— Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/3892#issuecomment-1012641481, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBER3VCUCT5LZYTGXWLK3UV5VBZANCNFSM4H3UYAYA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

klonos commented 2 years ago

I also agree that this is "easy" (in quotes because you first need to know what's changed or how to find what's changed), but you know what's easier than easy? ...everything working OOTB without having to change (almost) absolutely anything. It's also easier to tell people "hey, this is deprecated, and here's a link with instructions on what needs to be done" than simply throwing errors and assuming that they will know where to go to find answers, and that on top of it all, we also think that that's "easy".

And it's not about contrib only. There's heaps of D7 code snippets in tutorials and how-to's out there, and I'm sure also in custom code. I'd rather have all that work effortlessly OOTB, or worse case gracefully log useful deprecation warnings, instead of "nasty" errors about missing functions that will give people the impression that things that worked in D7 don't work in Backdrop. It's more subtle/graceful to have deprecation warnings logged instead of "things not working". Right? Then we can deprecate this completely in 2.0.

I fully understand the reasoning behind removing the theme hook, but we can do it in a better way in 1.x. Sure, there's performance concerns, but removing the function altogether is like trying to kill a mosquito with a cannon.

I don't see the need for a wrapper since we've got lots of places where contrib module maintainers need to update code.

I completely disagree with this statement. The reason for introducing watchdog_deprecated_function() was to make life easier for people that are porting their contrib/custom code to Backdrop (especially less experienced people). We can't simply dismiss this by saying "they are already having a hard time, might as well have some more".

bugfolder commented 2 years ago

I'm on team wrapper/deprecation notice here. I would bet that a lot of people porting modules will just run them through Coder Upgrade and if there's no immediate smoke and flames, release the module; or if it crashes with "unknown function" they might just throw up their hands and say "well, I tried, guess I'll go do something else." (Of course, an issue/PR to CU that automatically does the function replacement would be welcomed.) In porting D7 code, there's the big gotchas—CMI, layouts—and then a lot of little things that are hard to keep track of. The wrapper/deprecation notice is a big help to those folks.

findlabnet commented 2 years ago

Do we have any user interviews or feedback supporting this statement?

bugfolder commented 2 years ago

There's observation: I've seen (and taken over maintenance of) modules that contained issues that had been reported in change records but hadn't been applied. And I've seen at least one instance of the "well, I tried, not doing anything more" in comments in the past (sorry, though, that I can't provide a link, this is recollection).

jenlampton commented 2 years ago

It's also easier to tell people "hey, this is deprecated, and here's a link with instructions on what needs to be done" than simply throwing errors and assuming that they will know where to go to find answers, and that on top of it all, we also think that that's "easy".

I'm not against wrapper functions. I'd much rather have theme_html_tag() back only as a wrapper than anything else described in this issue. But I also think we have much more important backwards compatibility issues than this one, and I'd love to see us all move our efforts to those too :)

https://github.com/backdrop/backdrop-issues/projects/7

klonos commented 2 years ago

Working on #5489, I wanted to avoid having to do this hacky/manual mix-of-HTML-and-php-concatenation thing:

array('data' => '<label for="edit-modules-' . backdrop_html_class($modules[$project['name']]->name) . '">' . $module_label . '</label>'),

...so I did this instead, which seems more appropriate/standard:

$module_label = array(
  '#type' => 'head_tag',
  '#tag' => 'label',
  '#value' => $module_label,
  '#attributes' => array(
    'for' => 'edit-modules-' . backdrop_html_class($modules[$project['name']]->name),
  ),
);

...and then did this:

array('data' => backdrop_render($module_label)),

Things work as expected 👍🏼 ...but I'm calling the thing a "head_tag", when it is an "html_tag" 👎🏼

jenlampton commented 2 years ago

@klonos you shouldn't be using that function if it's not a head tag. That is likely to present problems in 2.x it/when we limit it's functionality so that it can only be a head tag.

If your intent is to make the installer functionality match the modules page, can you copy how the code was done for the modules page also? I'm sure it didn't use head_tag for a label.

argiepiano commented 2 years ago

I'm on team wrapper/deprecation notice here. I would bet that a lot of people porting modules will just run them through Coder Upgrade and if there's no immediate smoke and flames

I was just bitten by this - porting a module and wondering why an entire column on a table was not showing. I think a wrapper/deprecation notice would be great.

klonos commented 4 months ago

I am still annoyed/demotivated each time I review PRs and see that we are manually crafting HTML. Things like this:

    $checkbox_id = backdrop_html_class('edit-name-fields-' . $key);
    $fields[$key]['title'] = array(
      'data' => '<label for="' . $checkbox_id . '">' . $field['title'] . '</label>',
      'class' => array('title'),
    );

I wanna comment with something like "We should really be using backdrop_attributes() for the for attribute there" ...but then think "Oh, what's the point" 🤷🏼

backdrop / backdrop-issues

[D8][DX] Form API: Backport D8 `html_tag` element changes #3892

Original report

Change record

Problem/Motivation

Proposed resolution

API changes