Problem/Motivation

Look at this code, it's an inconsistent mess of title_prefix, title_attributes and label!

<div{{ attributes }}>
  {{ title_prefix }}
  {% if label %}
    <h2{{ title_attributes }}>{{ label }}</h2>
  {% endif %}
  {{ title_suffix }}

  <div{{ content_attributes }}>
    {{ content }}
  </div>
</div>

Proposed resolution

The fix: The variables printed in a template file for a title of a block should to be called title.

User interface changes

- none

API changes

- none

#1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer
#1591830: Change comment "subject" so that it's called a title like everything else in the template file (and all other template files)
#1825216: Name variables consistently across all templates (preprocess)
#1939234: Change node "label" so that it's called a title like everything else in the template file (and all other template files)

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task
Issue priority Normal because nothing is broken.
Unfrozen changes Unfrozen because it changes templates which are not frozen.
Prioritized changes The main goal of this issue is to improve themer experience by making variables consistent.
Disruption The noticeable change here affects templates, which should have minimal disruption.
Files: 
CommentFileSizeAuthor
#9 block-1939224-9.patch6.46 KBbostonid
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,249 pass(es). View

Comments

jenlampton’s picture

Issue summary: View changes

messy

jenlampton’s picture

Title: Change block "label" so that it's called a title like everything else in the template file » Change block "label" so that it's called a title like everything else in the template file (and all other template files)

just to drive the point home... tilte change.

jenlampton’s picture

Issue summary: View changes

comment

jenlampton’s picture

Issue summary: View changes

node

xmacinfo’s picture

We really do not need $block->label. At the minimum we should have $block->title.

But for all of us themers and developers, we should stick to $title.

xmacinfo’s picture

Issue summary: View changes

less snark

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

I'm all for changing label to title and getting this consistent. Updated the summary to show the current twig and some other minor changes.

akalata’s picture

At what point does it make sense to change the entity-provided "label" to the theme-friendly "title"?

joelpittet’s picture

@akalata Very good question, maybe worth asking in IRC #drupal-contribute to someone working on the entity system.

bostonid’s picture

Status: Active » Needs review
FileSize
3.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,255 pass(es). View

First contribution attempt. Apologies if way off target. Comments/advice appreciated.

Cottser’s picture

Status: Needs review » Needs work

Hi @simetau, thanks for your contribution!

Block UI uses Title, so this change seems rather valid to me even if it's inconsistent with the internal entity properties.

The changes overall look good to me, but we need to be careful to update all the block files that use or document label/title. I think only one might be missing but haven't looked very thoroughly:

/core/themes/classy/templates/block/block--system-menu-block.html.twig (it's documented at the top in the docblock)

Also we should update the docblock at the top of the templates to change label to title (but only for the "top level" label). This is where it might make sense to also have the documentation state that 'title' is equivalent to 'label' in internal block speak.

On a related note I'm mystified as to why block--system-menu-block.html.twig uses configuration.label instead of label. Probably needs a code comment (separate issue I'd say).

bostonid’s picture

FileSize
6.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,249 pass(es). View

Thanks @Cottser for the feedback and patience. I have updated 'label' to 'title' in the docblock at the top of template files (stole your 'internal block speak' line - wasn't sure on the voice) along with another comment in block.module.

I'm not sure on the other references to 'label' further down in the configuration area of the docblocks.

As always, advice and feedback needed and welcome.

bostonid’s picture

Status: Needs work » Needs review

Needs review.

The last submitted patch, 7: block-1939224-7.patch, failed testing.

isntall queued 7: block-1939224-7.patch for re-testing.

davidhernandez’s picture

@Cottser, the template first showed up here, #1869476: Convert global menus (primary links, secondary links) into blocks. Wim's patch in #153. I didn't read through the issue though so I don't know the explanation. 'configuration' existed before that though.

davidhernandez’s picture

Status: Needs review » Needs work

Check if these also need changing:

core/modules/comment/templates/field--comment.html.twig
core/modules/system/templates/field.html.twig
core/themes/bartik/templates/field--node--field-tags.html.twig
core/themes/classy/templates/field/field--comment.html.twig
core/themes/classy/templates/field/field.html.twig

Form element seems weird too:
core/modules/system/templates/form-element.html.twig
core/themes/classy/templates/form/form-element.html.twig

We have a title_display variable?

equivalent to 'label' in internal block speak

Is there a reason why block uses label and title inconsistently? I see both being used in various places.

Cottser’s picture

Thank you both. Just quickly I think this issue should only touch block templates, other templates should be handled separately IMO.

davidhernandez’s picture

Issue summary: View changes

@Cottser, that's fair; considering it is in the issue title. :)

I don't know if the documentation gods would approve of the wording of the parenthetical comment. That would be my only nit.

I'm removing the other stuff from the issue summary and adding beta evaluation.

googletorp’s picture

I'm not sure about this being an unfrozen change.

According to the documentation page, unfrozen changes are:

• CSS
• markup
• translatable strings
• documentation
• automated tests

This change goes beyond that, since it's also change twig template variables. I guess the proper way of going about this is add a new variable and deprecate the old. If I'm correct about this, this should be postponed to 8.1.x

davidhernandez’s picture

@googletorp, we should be fine. We are still making occasional template variable changes, but this will have to be done before release candidate (RC1). Then it will be frozen. After that, I don't think we can push it to 8.1. It would have to go to 9.x, because we can't really make backwards compatible changes to templates.

googletorp’s picture

We could push to 8.1.x by not deleting the old variable right away, but only deprecating it.

If it's general practice that template variables are OK to change, I guess this is fine. I wondered since this would break existing themes (sites) using this variable in the block template file.

Cottser’s picture

Hmm, I do kinda like the idea of keeping label as a BC thing, as long as we comment it accordingly.

davidhernandez’s picture

It isn't just an issue of the adding a new variable. We can't change the markup for the template in 8.1, which negates any reason to add another variable or change its name.

googletorp’s picture

We wouldn't change the markup, that would still be the same. The only thing we would change is the name of the variable. The actual output of the variable would be the same as the old. So there wouldn't be any change, instead we would have two variables for the same thing, until the deprecated one was removed.

But like I said, if there is a practice for doing this kind of thing for beta, then this is pretty irrelevant.

davidhernandez’s picture

We would be changing which variable is printed in the template, but the code of the templates themselves is frozen through the life of the major release.

It doesn't seem to make much sense though to live with an extra variable just for this. It's not hurting anything to leave it until it is fixed properly.

davidhernandez’s picture

Cottser and I just did some talking on this. We'll get a committers feedback to see what we can do.

davidhernandez’s picture

Status: Needs work » Postponed

We talked to webchick and alexpott in irc. My impression is everyone agrees the whole inconsistency problem all over core with how label versus title is handled is a big problem. But, it was discovered this particular change was done on purpose. See here, #1642070: Make use of entity labels in templates.

So, if we tried to fix this we should be switching everything (title_attributes) to use label instead, unless we want to have a discussion about preferring title (which no one wants to have.) The actual breaking change of renaming the theme variable seemed to be less of a concern.

Given the amount of work it would take to switch the title usage to label (not just in block), and the even greater number of things we'd be breaking, this is unlikely to happen in 8. We should probably push this to 9.x. @Cottser do you agree, or prefer a different course?

@googletorp, thanks for the diligence.

@simetau, I want to find something else for you to work on. (maybe something that isn't 2 years old. :P ) If I find a good issue I'll leave you message in irc.

Cottser’s picture

Version: 8.0.x-dev » 9.x-dev

Yeah, this seems too disruptive for beta/8.0.x and would be a breaking change in 8.1.x so sadly bumping the version. We need to approach this confusion and WTF-ness from a holistic place.

Thanks all.

xmacinfo’s picture

Version: 9.x-dev » 8.0.x-dev

Let’s not jump too quickly please! We have not reached RC yet. And even beta to beta upgrades are not currently possible, unless using the head to head project.

Why can’t we have $title variables in the TWIG templates is confusing. We do not want to print the label of a field, but the value of the field. So for a node title themers will want and prefer $title.

The idea of supporting both $title and $label in the 8.0.x release is much better; it'll be easier to remove the extra $label in 8.1.x.

Can we at lease see the point of view of maintainers?

davidhernandez’s picture

Version: 8.0.x-dev » 9.x-dev

@xmacinfo, I did outline some of this reasoning in comment #25. Also, see the issue I linked where this change to have label was purposefully introduced by a consensus of core themers. Cottser and I discussed this at length with webchick and alexpott who are both core maintainers, and Cottser is one of the theme system maintainers.

You may also be confusing the issue a bit. This isn't preventing anyone from printing the value of fields. This is only about the naming of the variable that is being used. No actual functionality would have been affected.

xmacinfo’s picture

No actual functionality would have been affected.

Agreed! But the perception is affected! :-)

catch’s picture

Version: 9.x-dev » 8.1.x-dev

I think we can do this with two variables, label being deprecated, and stable, in a minor release.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.