Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Oh, yes please.

jenlampton’s picture

Status: Active » Needs review
FileSize
17.77 KB

patch.

Status: Needs review » Needs work

The last submitted patch, block_titles_not_subjects-1591806-1.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
18.82 KB

trying again

Status: Needs review » Needs work

The last submitted patch, block_titles_not_subjects-1591806-4.patch, failed testing.

David_Rothstein’s picture

Related issue: #6234: Block module variable naming convention cleanup.

Yeah, it's been around for a while :)

ryan.gibson’s picture

Assigned: Unassigned » ryan.gibson
Status: Needs work » Needs review
FileSize
18.92 KB

Here's my swing at this.

Status: Needs review » Needs work
Issue tags: -Novice, -consistency

The last submitted patch, title_instead_of_subject-1591806-7.patch, failed testing.

stoickthevast’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +consistency

The last submitted patch, title_instead_of_subject-1591806-7.patch, failed testing.

ryan.gibson’s picture

Here's the second go. @timplunkett did the heavy lifting with this one.

Here's the thing, in the original code - title and subject both already existed. So, just changing the variables from subject to title didn't work. You can see in the interdiff what Tim did to work around it.

I'm thinking another solution would be changing title to something else, like "system_title" and then subject to "title". That'd take db update.

ryan.gibson’s picture

Assigned: ryan.gibson » Unassigned
Status: Needs work » Needs review
tstoeckler’s picture

What is currently 'title' is only used in the administrative interface, so maybe rename that to 'label'?

ryan.gibson’s picture

Custom blocks use "title" in the DB.

David_Rothstein’s picture

Actually all blocks use 'title' in the DB, not just custom blocks. (The block module provides a user interface allowing any block's title to be overridden.)

Given that, I wonder if it would make sense to instead rename 'subject' to something like 'default title', and avoid the conflict that way?

tim.plunkett’s picture

Status: Needs review » Needs work

Sure, I think renaming "subject" to "default title" makes a lot of sense. It also helps clarify that when using hook_block_view_alter(), you should change the title and not the subject, since the latter is overridden by the setting in the UI.

ryan.gibson’s picture

Assigned: Unassigned » ryan.gibson

I'll make the changes and post the patch in a bit during core office hours.

ryan.gibson’s picture

Assigned: ryan.gibson » Unassigned
Status: Needs work » Needs review
FileSize
16.1 KB

Okay, made those changes. Since 'subject' doesn't have a table in the database this won't require a db update.

tim.plunkett’s picture

Now the title no longer matches the patch. And since we're not switching it to title, does this really accomplish anything other than making D7 and D8 more divergent?

David_Rothstein’s picture

Well, the benefit would still be that 'subject' makes very little sense, but 'default title' conveys that it's actually a title.

However, maybe it's true that this wasn't a great idea after all. Because looking at the patch, I'm noticing things like this:

+++ b/core/modules/block/block.tpl.php
...
-<?php if ($block->subject): ?>
-  <h2<?php print $title_attributes; ?>><?php print $block->subject ?></h2>
+<?php if ($block->default_title): ?>
+  <h2<?php print $title_attributes; ?>><?php print $block->default_title ?></h2>

By the time you reach the theme layer, the actual title should have already been substituted in, so at this point it should be $block->title anyway, not $block->default_title.

We could do the variable switch at the point the substitution happens, and it would be nice and accurate that way, but perhaps it's simpler after all to just find a way to make it be 'title' all along.

tim.plunkett’s picture

Title: Change block "subject" so that it's called a title like everything else » Change block "subject" so that it's called a title like everything else on the theme layer

Good point about the template file.

podarok’s picture

Issue tags: -Novice, -consistency

Status: Needs review » Needs work
Issue tags: +Novice, +consistency

The last submitted patch, default_title_not_subject-1591806-18.patch, failed testing.

Risse’s picture

Based on #18, should work using latest dev.

Risse’s picture

Status: Needs work » Needs review

Let's change the status too :)

Status: Needs review » Needs work

The last submitted patch, default_title_not_subject-1591806-24.patch, failed testing.

Risse’s picture

Alright, new try, still based on #18

Risse’s picture

Status: Needs work » Needs review
acmaintainer’s picture

Please ignore. Posted by mistake.

kmcculloch’s picture

I downloaded this patch to test it on my local clone of 8.x, and it failed to apply because one of the underlying files, core/modules/block/lib/Drupal/block/Tests/BlockTest.php, had changed--the t() function calls on lines 106 and 107 had disappeared. So I edited the patch and it now applies okay on my clone.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#30 looks working for me
let`s make it RTBC

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Nice! :) I love this patch!

However, it no longer applies. Can we get a quick re-roll?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
15.7 KB

Rerolled! 33 insertions, 33 deletions, 33rd comment.

chanderbhushan’s picture

#33: drupal-1591806-33.patch queued for re-testing.

pakmanlh’s picture

FileSize
15.74 KB

patch works ok but the path of block.tpl.php was changed, this other solved it. Cheers!

Finn Lewis’s picture

Applied the patch from #35 without problem, and core block titles appear as expected.

psynaptic’s picture

I have reworked this based on the comments in #16 and #20 so we have $block->title in block template files. I have despised this about blocks for a long time and it's about time I did something to help fix it.

The logic goes:

* If the title is set in the UI to <none> then use an empty string as the title.
* If the title is set in the UI to anything but an empty string, use it as the title.
* In all other cases, fall back to the module-defined default_title.

The downside to this approach is that module developers will be able to use $block['title'] as well as $block['default_title']. Perhaps a positive side effect of this is that when module developers use $block['title'] it will become immutable from the UI. I know I have been in the situation in the past where I wanted to stop site editors changing the title of a particular block, but this may be a source of confusion if developers inadvertently use $block['title'] when they really want the behaviour provided by $block['default_title'].

psynaptic’s picture

More accurate commit message.

psynaptic’s picture

Come to think of it, if the site editor tried to change the block title and it didn't change they would likely file a bug report and we don't want to have to deal with a flood of bug reports for something we think is a feature. Besides, if you really want to disable the title then it can be accomplished by disabling the title field in the form, or hiding it, or whatever.

psynaptic’s picture

I guess we need to decide if we're ok with letting $block['title'] in hook_block_view or hook_block_view_alter be immutable. If not, I'll have to think of another approach because I'm dead set on having $block->title in template files!

Fabianx’s picture

Issue tags: +Twig

Slightly related to twig initiative.

RobLoach’s picture

Issue tags: -Twig

Looks great!

+++ b/core/modules/block/block.module
@@ -932,18 +932,27 @@ function _block_get_renderable_block($element) {
+    if ($block->title == '<none>') {
+      $title = '';
+    }
+    elseif ($block->title) {
+      $title = check_plain($block->title);
+    }
+    else {
+      $title = $block->default_title;
     }
+    $block->title = $title;

Could probably directly set $block->title here instead of using that temporary $title variable. I'll do a quick re-roll.

RobLoach’s picture

Issue tags: +Twig
FileSize
17.34 KB

Quick re-roll.

c4rl’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

chx’s picture

Except we just changed to node label instead of node title which contradicts the issue title.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Yeaaahhh.. :\

And actually. I wonder if we change this to straight-up $label if that doesn't solve the entire issue. Then we wouldn't need this weird "default" prefixing as psynaptic pointed out in #40.

Really sorry to do this folks, but I think we still need to work on this a bit.

steveoliver’s picture

chx, you mean issue label ? ;). I think title makes much more sense than label. Title is the one single title. Logically to me, a "label" can be one of many labels. -1 on node "label". Issue link?

steveoliver’s picture

Ah, I see.. #1616962: Replace $node->title with $node->label() -- label method instead of direct title property.

c4rl’s picture

It appears that label() is a method on entities and the title property still exists (as described here http://drupal.org/node/1697182).

Using default_title seems okay to me because:

(1) Blocks aren't entities.
(2) It is indeed a default because we can override it via the block configuration form.

What are some alternative ideas? If we were to use 'label' as suggested in #46, what is immutable and what is not? I agree that $block->title should exist in tpls as suggested in #40 (i.e. {block.title} in twig-speak).

psynaptic’s picture

As long as we have consistency I don't mind which we pick.

I thought that "title" had good semantics for the template variable name since that's what we most commonly used it for in nodes, blocks, and so on. However, I can see some places where that definition would break down. Perhaps we should adopt "label" in all template files so that we're not making any assumptions as to its usage? I would prefer not to have the divide between backend and frontend using $node->label() vs node.title.

@c4rl: One of the main motivations for me to work on this issue was that I really disliked having different names for accessing what I thought of as a "title"; I just wanted everything to be consistent and use the same naming conventions. If that means switching to "label" then I can live with that.

Thoughts?

c4rl’s picture

@psynaptic: I agree that consistency should be the primary goal, certainly any revisions and progress here should support that foremost. I'll ponder this a bit more to that end. Naming things is difficult. :)

jenlampton’s picture

Issue summary: View changes

related

jenlampton’s picture

Status: Needs work » Reviewed & tested by the community

I think if we are going to change the title on a node to call it a label (is it called a label in the UI too? It still looks like a title to me!) then we need to change everything else that had a title (or a subject) to have a label instead. I don't really care what we call it, but we need to be consistent.

I would love, love, love it if we could have a "Title" in the UI, a node.title in the templates, and a node->title in the code, and if all thingies (nodes, blocks, comments, form elements?) could use the same pattern. (I would also be willing to switch to Label or anything else for that matter if we could accomplish this)

The path forward with the least amount of work, is to stick with title since *almost* everything has a title already. That's also the best DX since all current Drupal devs are used to most things having titles. If we can change comment to have a title too (see #1591830: Change comment "subject" so that it's called a title like everything else in the template file (and all other template files), and change the variable back in node templaes (see [#], then we'll at last be on the same page with everything - for once :)

Since it looks like nodes still have titles, I'm inclined to stick with title here too. Changing status back to RTBC.

psynaptic’s picture

I agree with everything that jenlampton just said.

xjm’s picture

(1) Blocks aren't entities.

Yet.

xjm’s picture

I'm also not sure this is the right direction. Custom blocks will eventually be configuration entities, which will have a label like everything else.
http://drupal.org/node/1739820
http://drupal.org/node/1697182
http://drupal.org/node/1739820

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Seems like this still needs more discussion?

psynaptic’s picture

I'm fine with label as long as we have the same name for it throughout.

xjm’s picture

This is from node.tpl.php, for example:

<?php print render($title_prefix); ?>
  <?php if (!$page): ?>
    <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>" rel="bookmark"><?php print $label; ?></a></h2>
  <?php endif; ?>
  <?php print render($title_suffix); ?>
lotyrin’s picture

Blocks aren't entities (at least at this point) so there's no label().

Even for entities, the underlying property that label() pulls from still exists ($node->title, $term->name for instance).

I don't see how entities moving to label callbacks affects this.

lotyrin’s picture

Issue tags: -Novice, -consistency, -Twig

#43: 1591806.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +consistency, +Twig

The last submitted patch, 1591806.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review

@lotyrin, see #58.

xjm’s picture

Also, $title is no longer available in the node template. See #1811684: XSS: Bartik's node.tpl.php prone to XSS (prints $title).

xjm’s picture

Reading the patch in #1535868: Convert all blocks into plugins really does make it clear how goofy "subject" is, one way or another.

xjm’s picture

Status: Needs review » Postponed

We need to postpone this on #1535868: Convert all blocks into plugins. I apologize for doing so, but that issue blocks like two whole initiatives, and followups to it will probably better inform this issue.

xjm’s picture

I think it would make sense to do this once #1871696: Convert block instances to configuration entities to resolve architectural issues is in; then we can properly get rid of "subject".

sun’s picture

Still postponed, but on #1871696: Convert block instances to configuration entities to resolve architectural issues now, I think. ;)

However, within the new architecture, there's a differentiation between a block plugin instance (which is what you place into a layout), and a block content (entity).

To be consistent with everything else, I think these two should use two distinct properties/keys for their respective headings (blahahah):

- Block plugin instances are configuration, so they should use $label.

- Block content entities are content, so they should use $title.

xjm’s picture

Actually content uses $label too: #1811684-10: XSS: Bartik's node.tpl.php prone to XSS (prints $title). (As stated in that issue, it's a mite confusing, but that's what other content entities do as well.) Also, the custom block's title is not likely to be exposed in the template, since it is essentially just a default title for instances of that block.

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
29.78 KB

This should be good to go, if it passes tests.

Completely different patch at this point.

Status: Needs review » Needs work

The last submitted patch, block-1591806-69.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
30.82 KB
1.04 KB

Missed a spot.

sun’s picture

  1. I'm very grateful for the removal of 'subject'. Long overdue!
  2. However, 'default_label' really does not mash up with everything else, and I think it's architecturally wrong, since, by design, everything in annotations can only represent default values. Therefore, a 'default_' prefix makes no sense. This should just be 'label', like any other comparable annotation property elsewhere.
  3. In fact, I'd actually go one step further and will say: IF a 'default_' prefix for any annotation property is even remotely needed, then that property has absolutely no business in the annotation and and it should be moved into a class method instead. That is, because there mere fact that there's a consideration of having to clarify the annotation property's purpose in order to distinguish it from an actual plugin setting pretty much means that the annotation property's value is almost exclusively used in situations in which the plugin is getting instantiated. Any info that isn't needed without/prior instantiation should NOT be part of the annotation.
tim.plunkett’s picture

I think the default_ prefix is still useful to help differentiate between the suggested label and the entity's actual label. At the very least it's easier for me to understand. I definitely understand your perspective on this, but in this case I think that DX trumps architectural purity.

I'm 100% agreed on moving things out of annotations, but we don't have a system for allowing things outside of annotations to be altered. The subject, or default label, was alterable in D7, so we've left it in there.

EclipseGc’s picture

so, the actual naming of things aside, I'm fine with the patch.

The naming of things is of course one of those hard things... I sort of feel like what you're calling "default_" right now is really more like "admin_". These things should be functioning as the administrative name for the block both before and after it is placed. I'd love to see that ultimately be overridable through the UI as well, multiple instances of the same block can end up quite confusing in the existing UI (sun can probably attest to this from the primary/secondary menus as blocks issue). Still... the label for this would have to be REALLY wrong before I'd get too up in arms. The change is definitely long over due, my proposal is admin_label, but I'm really fine with whatever.

Eclipse

tim.plunkett’s picture

FileSize
30.73 KB

Renamed to admin_label. I don't really care, as long as it is *_label. (Not 'subject', not 'label').

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

I'm completely cool with this, rtbc assuming it passes tests. Let's get at least this in and then argue over further changes if necessary unless there's just something really really wrong here. Breaking this up so that both aren't referred to as subject has to be a huge win though.

Eclipse

tim.plunkett’s picture

Yeah, I'd love a follow-up along the lines of #72, but I feel like that's related to #1764380: Merge PluginSettingsInterface into ConfigurableInterface

tim.plunkett’s picture

#75: block-1591806-75.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +consistency, +Twig, +Blocks-Layouts, +Plugin system

The last submitted patch, block-1591806-75.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
30.44 KB

Rerolled. The Views block comment fix was already in the views code, so that is why the patch it a tiny bit smaller.

Gábor Hojtsy’s picture

Issue tags: +Spark

Also tagging as something followed by Spark. Arrived here with the chain of dependencies, eg. my confusion in #1875260: Make the block title required and allow it to be hidden about the mix of label/subject terminology.

Status: Needs review » Needs work

The last submitted patch, block-1591806-80.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
31.06 KB
632 bytes

There was one legitimate test fail due to the XSS block test block plugin. The other one was a fluke.

Status: Needs review » Needs work
Issue tags: -consistency, -Twig, -Blocks-Layouts, -Plugin system, -Spark

The last submitted patch, block-1591806-83.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +consistency, +Twig, +Blocks-Layouts, +Plugin system, +Spark

#83: block-1591806-83.patch queued for re-testing.

vmunoz’s picture

After applying the last patch, I'm getting these NOTICE messages when loading 'admin/structure/block' page:

Notice: Undefined index: admin_label in Drupal\block\BlockListController->buildForm() (line 141 of core/modules/block/lib/Drupal/block/BlockListController.php).
Notice: Undefined index: admin_label in Drupal\block\BlockListController->buildForm() (line 152 of core/modules/block/lib/Drupal/block/BlockListController.php).
Notice: Undefined index: admin_label in Drupal\block\BlockListController->buildForm() (line 159 of core/modules/block/lib/Drupal/block/BlockListController.php).

Gábor Hojtsy’s picture

@vmunoz: that should be due to your existing config lying around. Please reinstall your site cleaning out the existing config files from your files dir. The patch would not pass tests if it would exhibit those notices.

Any other review feedback now that it passes again?

tim.plunkett’s picture

#83: block-1591806-83.patch queued for re-testing.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

looks like no significant changes since I last said RTBC, so...

Gábor Hojtsy’s picture

Issue tags: +sprint

Add for Spark sprint tracking.

webchick’s picture

Title: Change block "subject" so that it's called a title like everything else on the theme layer » Change notice: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

admin_label is still not ideal, but at least until/unless #1875260: Make the block title required and allow it to be hidden is sorted out, this works. "subject" doesn't make sense in the least, at least admin_label is descriptive. ;)

Committed and pushed to 8.x. Thanks!

Marking for change notice.

Gábor Hojtsy’s picture

Title: Change notice: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer » Change block "subject" so that it's called a (admin_)label like everything else on the theme layer
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

There is no theme facing change in this commit, only internal admin label changes for what block plugins set up. The subject/title/label problem is in dire need of being resolved for blocks both at the API level and the theme level still. This issue just untangled one use of "subject" from the other use of "subject", so the admin label targeted default titles are now admin labels while the edited labels are still "subject"s.

So I've updated the change notice at http://drupal.org/node/1880620 (diff at http://drupal.org/node/1880620/revisions/view/2527496/2589882). I don't think we add individual change notices for internal changes in newly introduced Drupal 8 APIs, we usually update existing change notices and documentation. That said I also looked at docs at http://drupal.org/node/1882526 but did not find block examples, there are other examples for plugin annotations. So no need to fix there.

I believe this should be considered complete.

jenlampton’s picture

No theme facing changes - that's a problem considering the title of the issue :/
I'll open another issue since this one seems to have gotten a bit sidetracked :)
#1939224: Change block "label" so that it's called a title like everything else in the template file (and all other template files)

Gábor Hojtsy’s picture

Issue tags: -sprint

Landed.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

related