Convert site elements (site name, slogan, site logo) into blocks

??

content is block in D7

mission and footer are already a block.
the rest is D8 material.

Michele,
Have you found Blockify module?

Major changes along these lines are in progress at http://drupal.org/sandbox/eclipsegc/1441840

This is a relatively small, focused issue, so it should remain open.

But see also #507488: Convert page elements (local tasks, actions) into blocks which is closely related (and has a patch). Although they focus on "blockify-ing" different things, it may make sense to merge them.

Although I forgot to change the issue title to reflect its actual focus :)

Tagging it up.

All right, so given #1535868: Convert all blocks into plugins, it probably does not make much sense at this point to convert these to blocks as blocks exist pre-plugins. However, @EclipseGC actually did lots of the conversion work that is involved with these blocks in interim patches at #1787846: Themes should declare their layouts. It was not actually related to introducing layouts as a concept, and I want to make sure we have the patch around, so here it is. All the code included is from @EclipseGC.

Blocks included:
- site name
- site logo
- site slogan

Technically this issue does not make much sense until #1535868: Convert all blocks into plugins however, I think it would make sense to parallelise work. This needs to be kept up to date with the API there.

#9: site-elements-as-blocks-7.patch queued for re-testing.

Have been looking at #1535868: Convert all blocks into plugins for ways to move this forward. I believe:

1. We'd need to introduce a new region to place these blocks (current header regions have a different setup).
2. We'd need to add the new block to the profiles in core as CMI configs. See below for help snippet from the patch.
3. We'd need to remove the template variables related to these blocks.

Config snippet to enable a block with a profile (in this case the help block for the bartik theme in the standard profile):

diff --git a/core/profiles/standard/config/plugin.core.block.bartik.help.yml b/core/profiles/standard/config/plugin.core.block.bartik.help.yml
new file mode 100644
index 0000000..a5c4467
--- /dev/null
+++ b/core/profiles/standard/config/plugin.core.block.bartik.help.yml
@@ -0,0 +1,18 @@
+id: system_help_block
+status: '1'
+cache: '-1'
+visibility:
+  path:
+    visibility: '0'
+    pages: ''
+  role:
+    roles: {  }
+  node_type:
+    types:
+      article: '0'
+      page: '0'
+  visibility__active_tab: edit-visibility-path
+subject: ''
+module: system
+region: help
+weight: '0'

@EclipseGC said in IRC we might not need to introduce a new region. Well, currently, the header region in bartik for example is on the right side of the header while logo and slogan and site name (compounded!) are on the left side:

Since we don't have any means to position (float) blocks in regions as we wish, I don't think an overall header region works. Unless we make the header region 100% wide, place the logo at the first place. Then introduce a "site name and slogan" compound block (not as two separate blocks) and place that by the side of the logo. So all blocks in the header region would be left floated in bartik.

Finally, I just realised we need to get rid of the theme settings for logo, site name and slogan display both on the global and per-theme level as well. That would need an upgrade path, however blocks have no upgrade path yet, so that upgrade path might need to go into #1871700: Provide an upgrade path to the new Block architecture from Drupal 7 eventually, not in this patch.

IRC review from EclipseGC on regions:

EclipseGc: GaborHojtsy: you how to on that look right to me, I think the new regions is debatable, but it's also the quickest way forward for some things. (primary and secondary menus for instance)
EclipseGc: GaborHojtsy: and your proposed way to do this w/o introducing new regions is exactly what I was expecting. Theme's are going to provide their own version of block or hook_block_view_alter() them, in which they should ultimately be able to add/change the css, and weight these things appropriately
EclipseGc: GaborHojtsy: but as I said, I'm also not opposed to adding new regions here

On the theme settings:

EclipseGc: GaborHojtsy: ok, so totally ++ on the removing of those settings

It looks like there are 3 things to be done: (1) make these elements (configurable)blocks, (2) determine which of the 3 core themes need to support these elements as blocks and modify appropriately, and (3) modify install so these blocks are configured into the core themes that will use them by default. Does all this have to be done in this issue? Shouldn't modifying the core themes to support these elements as blocks be a separate issue for each theme?

4.) We may have some caching issues depending on how themes handle this.

Eclipse

@dcrocks: well, unless we modify themes, the blocks will be there but not used/useable for anything. Not sure what value does a patch committed on that level of usefulness has => hard to argue to break it down to that much IMHO. If we consider theme changes should be their own followups, then the extent of the change is already in the above proposed patch.

Not saying to postpone anything. I would think a theme related follow-up would be immediate. It just seemed to be a clearer discussion and, almost, a way to self document the change. And, as soon as these blocks are available, I expect people would use them. I would/will.

@dcrocks: as demonstrated on the screenshot in #13, it is not possible to place a logo or site name block where the logo or site name is in bartik, so using it as soon as available (for its main purpose) is not possible without modifying the theme.

Does not the existing code in bartik work after these elements are made into blocks? Are the variables removed? What happens to seven and stark and the core system templates? I didn't think this patch broke any existing themes.

@dcrocks: did I say that? AFAIS all I said is this does not make the blocks usable because the themes provide no place for them to be put in their primary role, at least there is no region there to use.

The existing patch only adds the blocks. If it is also going to modify bartik or any other core code, perhaps the issue title should be changed

#9: site-elements-as-blocks-7.patch queued for re-testing.

Cannot #9 be committed as it stands? I tested it in 8.x-dev dated 1/18 with no ill affects. Committing it had no affect on existing core themes. With it in people can start using it for 8.x themes. A new issue to have bartik take advantage of this could be started separately. In the attached image I assigned them to sidebar 2 in bartik but made absolutely no changes to bartik, just to see what it would look like.

ps. I had to enable the layout module to see these. Why aren't they built as system plugins instead of layout plugins? They are system data aren't they?

Actually, they have to be in a module enabled during standard install, else they won't be available to bartik at install. System module seems the logical place.

The blocks indeed seem to be more logical to be exposed by system module where the related settings are. Definitely not in layout module.

Before another patch is created, does it sound reasonable that some default styling be done on these blocks in core? They are not ugly, even in stark, but some standard default appearance could be given to them.

1. Can we introduce a 'branding' region as one of the default regions? Seven implements that already (in HTML markup, not as region). There's almost no theme that hasn't a branding region (except, Seven, actually, *g*), so it would be sane to introduce that as a default region.
2. Styling should be left to individual themes. If we want to discuss "block styles", then we'll quickly start to duplicate the discussion that happened in #503810: Convert primary and secondary menus to blocks already — essentially putting the basics of Skinr into core; (1) Registering class names as styles, and (2) allowing to assign styles to blocks. We should probably file a dedicated issue for that topic, since that would be the complementing counter-part to layouts. (Layouts control placement, Styles control appearance.)

@sun: branding region sounds nice.

Currently there are styles for a site name (bigger font) or slogan (italics in some cases), so not sure if you are suggesting to remove all those altogether (eg. you could only place the site name in the regular page font size/type and not reproduce the above screenshot where it is with a larger/different(?) type). That sounds like a regression.

just noting that this is a blocker for SCOTCH, and the work i'm currently doing in our princess branch.

it may be that this is easier to finish up in princess directly because we don't have to deal with some of the architectural awkwardness (like special regions). if that's the case, i may just pull the base work in there and go forward with it.

This is another one of those issues that it'd be great to have as a patch separate from the princess branch, because it has value outside of SCOTCH.

#9: site-elements-as-blocks-7.patch queued for re-testing.

Portland Code Sprint is wrapping up. Here is a quick port of the patch in #9 to D8, moving plugin to system.

The patch looks ok, however the 'subject' property in the class anotation has been renamed to 'admin_label'.

Also, it looks like the test bot is having problems.

EDIT: We should probably alter Bartik to use these blocks instead of just printing the items out.

I think the test bot problems are from the overload at the sprint. I suspect it will be fine the next time it runs.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/PageSiteLogo.php
@@ -0,0 +1,34 @@
+ *   subject = @Translation("Site Logo"),

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/PageSiteName.php
@@ -0,0 +1,30 @@
+ *   subject = @Translation("Site Name"),

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/PageSiteSlogan.php
@@ -0,0 +1,35 @@
+ *   subject = @Translation("Site Slogan"),

These are the lines that should have "subject" replaced with "admin_label".

"subject" changed to "admin_label"

When trying to place the block:

Fatal error: Class Drupal\system\Plugin\Block\PageSiteLogo contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\block\BlockBase::blockBuild) in /Users/jfallon/Sites/d8/core/modules/system/lib/Drupal/system/Plugin/Block/PageSiteLogo.php on line 20

So, there is more work to do.

Why do u use children and not #markup?

Changes needed so that placing blocks to Bartik header region does not generate errors.

Updated issue summary.

aspilicious: Can you make a case for #markup over #children?

The goal in the latest patch was just to make the patch from comment #9 work in D8.

I have been working on #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) and I think it would be easier to have the site name and the site slogan handled by the same block.

They have to be seperate blocks imo

I was talking in IRC with @lokapujya about how to address the issue of getting the site name and site slogan to line up the way they did before. We see a few solutions:
The first is to make them into a single block, however, this would limit the amount of layout customization one has.
The second would be to add even more regions to the header. If we go with the add more regions route we could end up with 6+ regions in Bartilk's header area.
A third option would be to just palce the blocks with CSS, however simple styling can easily be broken by just moving some blocks around or having a really long site name, thus requiring dozens of selectors to try and catch all of the likely scenarios. The layout module looks like it might be a good solution for this at some point, however it is still far from complete.
In short, we still don't know what the best option is(not helpful, I know :P).
If anyone has any ideas that would be great.

I don't think we can make any assumptions about site design, what if I want my slogan in the footer?

The theme should decide how different blocks layout within a region. It's a tricky problem to deal with in core or contrib themes but it's no different to the situation now in D7.

#42: 1053648-site-elements-as-blocks-8_1.patch queued for re-testing.

Adding sub-task for Bartik theme.

Simplifying scope of this issue.

The text in the site name block needs to be a link to the home page.

Removing the unnecessary B&L tag.

#42: 1053648-site-elements-as-blocks-8_1.patch queued for re-testing.

I think we're are missing a really valuable point in Drupal 8 - that a block can have as many instances as you want.

So, we could do it all in one block - logo, site name and slogan. Have configuration in the block that allows you to choose which element you want 'in this instance of the block'.

We could also rather easily write classes into the block attributes that tell us what bits are being used (just a suggestion, personally I would do this).

Get the best of both worlds, for Bartik and themes that do not want to laden themselves down with screeds of CSS "what ifs" get the advantage of being able to lock down the design and save site builders from (easily) breaking their theme.

For those cases where these things need to be reordered or have some edge case requirement like slogan in the footer Drupal supports that capability already. Just use another instance of the block and turn off what you do not want.

This does not really take anything away from the flexibility but allows us themers to actually produce designs that will work robustly and not "way to easily" broken by the site builder (and who may not even understand why or how their theme/site got broken).

I think this could be a workable solution. Thoughts?

Sounds like a good approach to me.

I think its a good approach also but I have to actually code this up and see how hard this is the theme, I mean get inside {{ content }} in the block (whether in a template/preprocess/hook_theme_prepare/alter etc) and reorder things in code, as long as thats reasonably easy I see not too many other issues with this approach.

OK, heres a proof of concept patch for discussion and review.

This harks back to #28/29 sort of but instead of providing a branding region we provide a branding block and allow site builders to toggle the elements they want in that particular block instance.

As discussed above this has the rather nice advantage of locking down the source order pretty tightly, which is either good or bad depending on how you want to look at this. I have approached this so you can change the order in code pretty easily, which you always had to do before anyway, just a different bit of code: e.g.: $variables['content']['branding']['sitename']['#weight'] = -1; would move the site name to the top.

Caveat, this is the first patch I ever wrote for D8 and the first one in years, so expect that I am out of touch and probably missed some docs, this is for discussion regarding the approach.

Tried it out, seemed to work fine. But, would prefer separate blocks for each content type just to make it easier to identify what is actually being displayed by each instance. As you can see in

There is no way to tell, or change, on the block admin page, what each instance is actually displaying. Contextual links are usable for seeing and displaying block instance configs but it is inconvenient not to be able to do the same on the block page.

PS. These instances are indeed displaying different content.

I think strictly speaking that might even be outside the scope of this issue, e.g. a broader UX issue that may or may not have been discussed already, mainly because this affects not only this block but views blocks, custom blocks etc. I'd say if we get a directive to include custom labels in block admin to show config values then we should do it. I would defer to the UX peeps on that particular issue. Basically the solution to the admin labelling issue is not to have these as separate blocks but to use CMI (I have not delved deep into BlockBase as yet).

#1956448: Use the block instance title on the block admin listing :)

Formatting Change.

57: 1053648-site-elements-as-blocks-8_2.patch queued for re-testing.

57: 1053648-site-elements-as-blocks-8_2.patch queued for re-testing.

Good idea! seems this branding needs some theming and css to actually move forward

57: 1053648-site-elements-as-blocks-8_2.patch queued for re-testing.

I tried the patch on a fresh clone of D8. The attached image shows sample output.It looks like it might be difficult to theme but I don't see how you can add theming thru this patch as you don't know where or how each instance will be used. If you look at a sample of the generated output

 <div id="sidebar-second" class="column sidebar"><aside class="section">
 <div class="region region-sidebar-second">
 <div class="block block-system contextual-region" id="block-sitebranding-2">
  <h2>Site Branding B</h2>
  <div data-contextual-id="block:block=sitebranding_2:"></div>
  <div class="content">
  <p class="site-slogan">Look Here</p>
  </div>
  </div>
  <div class="block block-system contextual-region" id="block-sitebranding">
  <h2>Site Branding A</h2>
  <div data-contextual-id="block:block=sitebranding:"></div>
  <div class="content">
  <h1 class="site-name"><a href="/~rocks/drupal8/index.php/" rel="home" title="Home">Drupal8</a></h1>
  </div>
  </div>
  </div>

You can see there is not much help for theming.

@dcrocks: Yeah, it looks like there should be at least a class name on the block that identifies the block delta, now that the same block can be used multiple times..

#66, #67 there is a unique ID added by Drupal core, if core wants to add a unique class per instance (which I support) that is a separate issue.

One think I mentioned earlier in the thread (#54) was adding configuration type classes, aka so you know what is active in the block, e.g. .has-logo, .has-sitename sort of thing. Thats something I have generally done on branding type wrappers in my themes, it can be useful, but also kind of edge casey, maybe in this system it would be more important since you are, in essence, having multiple branding type elements (blocks).

Basically we'd have to get that info from config/add the classes in preprocess, not sure that is ideal for core from a general performance perspective, I personally think that those classes could be edge cases and perhaps it's better to leave that to the theme to implement - at least that seems to be the general direction core is heading in - aka a far more minimal approach to markup out of the box, which is why this patch is conservative and does not include special casing for classes.

@Jeff Burnz: I wasn't suggesting a unique class, but one that identified the block plugin. There are multiple blocks on the page from the same plugin, but no class to tie them together (other than block-system), so you can't apply the same CSS to them all. Does that make sense? Sorry, if I've been unclear.

I'm picturing something like:

<div class="block block-system block-system-branding-block contextual-region" id="block-sitebranding-2">
</div>
...
<div class="block block-system block-system-branding-block contextual-region" id="block-sitebranding-3">
</div>

I added the "block-system-branding-block" (based on the plugin ID of "system_branding_block") so that you know that each of those blocks represent the same block plugin.

But, yes, I agree this is a seperate issue. :-)

#69 ah right, there is an issue for that, I'll dig around and see if I can find it, we argued/discussed this at length some time ago which stalemated over the block- prefix.

This is the issue I think is worth looking at regarding plugin class: #1896098: Add a plugin class to the blocks to identify instances in css

Site name should be check plained, so now uses String::checkPlain()
BEM'd the classes.

One thing we should note is that this patch does not account for the Toggle theme settings, which became rather ambiguous - so Site name, Slogan toggles would be removed, and Logo toggle modified to account for there being no visibility toggle within the theme settings, just a file field to override the themes default logo. Should we add/do this here, or not?

@Jeff Burnz: Thanks for finding the issue! :-)

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,115 @@
   +    $site_config = \Drupal::config('system.site');
   

   Config should be properly injected

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,115 @@
   +      $slogan = filter_xss_admin($site_config->get('slogan'));
   

   Use Xss::filterAdmin()

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,115 @@
   +        $image = '<img src="' . $logo['url'] . '" alt="' . t('Home') . '" />';
   +        $build['branding']['logo'] = array(
   +          '#markup' => l($image, '', array('html' => TRUE, 'attributes' => array('rel' => 'home', 'class' => array('logo'), 'title' => t('Home')))),
   ...
   +        '#markup' => l($sitename, '', array('attributes' => array('rel' => 'home', 'title' => t('Home')))),
   ...
   +        '#suffix' => '</h1>',
   ...
   +        '#prefix' => '<h1 class="site__name">',
   ...
   +          '#markup' => $slogan,
   +          '#prefix' => '<p class="site__slogan">',
   +          '#suffix' => '</p>',
   

   Is there a way to use proper render array?

#74

"Config should be properly injected", not sure what is meant by this, I copied whats in theme.inc preprocess for now, is there an example I can follow?

"proper render array" - yeah, I sort of get what you mean, this stuff always seems so confusing to me, what to do where etc. I assume you mean don't call theme l() here and do a full proper render array for the whole lot, if so, yes, I agree.

This may help with the "proper render array" thing...

        '#type' => 'link',
        '#title' => $sitename,
        '#href' => '<front>',
        '#attributes' => array('rel' => 'home', 'title' => t('Home')),

Config maybe should be injected but none fo the base classes seem to have it already injected, so from a pain in the ass point of view... this is very likely how it will be done in the wild:) However, translation and the t() function are already injected for you so you can use $this->t().

There is a $this->configuration, but I don't think that is the same, that looks like the block configuration and not the config factory from CMI.

I'm trying to formulate a plan in my head to avoid markup being used in #prefix/#suffix because that gets abused too much but so far I've got nothing, so until a brilliant idea come about, carry on:)

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
@@ -0,0 +1,115 @@
+    $build = array();
+    $site_config = \Drupal::config('system.site');
+    // Logo
+    if ($this->configuration['logo'] == TRUE) {
+      $logo = theme_get_setting('logo');
+      if (!empty($logo)) {
+        $image = '<img src="' . $logo['url'] . '" alt="' . t('Home') . '" />';
+        $build['branding']['logo'] = array(
+          '#markup' => l($image, '', array('html' => TRUE, 'attributes' => array('rel' => 'home', 'class' => array('logo'), 'title' => t('Home')))),
+          '#weight' => 0,
+        );
+      }
+    }
+    // Site name
+    if ($this->configuration['sitename'] == TRUE) {
+      $sitename = String::checkPlain($site_config->get('name'));
+      $build['branding']['sitename'] = array(
+        '#markup' => l($sitename, '', array('attributes' => array('rel' => 'home', 'title' => t('Home')))),
+        '#prefix' => '<h1 class="site__name">',
+        '#suffix' => '</h1>',
+        '#weight' => 1,
+      );
+    }
+    // Site slogan
+    if ($this->configuration['siteslogan'] == TRUE) {
+      $slogan = filter_xss_admin($site_config->get('slogan'));
+      if (!empty($slogan)) {
+        $build['branding']['siteslogan'] = array(
+          '#markup' => $slogan,
+          '#prefix' => '<p class="site__slogan">',
+          '#suffix' => '</p>',
+          '#weight' => 2,
+        );
+      }
+    }

If this is to be themed, it should likely be a twig template.
core/modules/system/templates/branding.html.twig Or something along those lines.

array(
  '#theme' => 'branding', 
  '#use_logo' = $this->configuration['logo'],
  '#logo' = $logo,
  etc...
)

  {% if use_logo %}
    <a href="{{ site_url }}" ...><img src="{{ logo.url }}" alt="{{ 'Home'|t }}" /></a>
  {% endif %}
  etc.

Thoughts?

Worked with timplunkett and joelpittet in IRC to create a revised version of this patch that addresses the above concerns, and which uses a twig template, which is much cleaner. Please note that we changed the class for the logo to 'site__logo' to match the pattern of the other branding elements.

Tagging.

+++ b/core/modules/system/templates/branding.html.twig
@@ -0,0 +1,22 @@
+  <a href="{{ url('<front>') }}" rel="home" class="site__logo" title="{{ 'Home'|t }}"><img src="{{ site_logo.url }}" alt="{{ 'Home'|t }}" /></a>
...
+  <h1 class="site__name"><a href="{{ url('<front>') }}" rel="home" title="{{ 'Home'|t }}">{{ site_name|e|t }}</a></h1>
...
+  <p class="site__slogan">{{ site_slogan|t }}</p>

I'm not able to review the rest of the patch but the use of double underscore's seem a little off. Our CSS standards specify the usage to:

define a component’s elements explicitly, prefixing them with the component’s name followed by two underscores

Which means a class like site__logo would mean there is a component called site that contains this element.

My suggestion would be just to stick with site-logo

Great, this is where it needed to go, the template is a vast improvement as is the rest of the patch. Class names should probably be changed as Lewis points out, my bad, I lumped in my own approach here (my earlier patch was cut from my own test site where I use this form of BEM, but it's not cores approach precisely).

I was not able to uncheck by default the "Display title" toggle in my hacking on earlier patches, is this actually possible, I would think no one wants to show a title?

I tested the patch on simplytest.me to see what you meant by the display toggle. That toggles the title of the block on and off: by default, it has the title Site branding.

I agree, I would be hard pressed to think of a reason why somebody would want to create a block for site branding elements with the title showing. I'm not familiar enough with the new block plugin system to know how to toggle that off by default, but maybe somebody else can point the way.

Changing the class names is very doable. Change them to "site-name", "site-logo" and "site-slogan"?

Just to clarify the roadmap, this issue is specifically to provide the site branding elements as an optional block, correct? Once that is in, would we create these sub-issues as followups?

• Create a branding region for themes, which would include the site branding block by default
• Remove the site branding elements from the page template in favor of the branding region
• Deprecate the site branding variables and remove the variables to turn them on and off on the appearance page

If that's the eventual roadmap, I would imagine we'd need to follow a similar process for menu items (#1869476: Convert global menus (primary links, secondary links) into blocks) and titles, tabs, actions and messages (#507488: Convert page elements (local tasks, actions) into blocks).

It looks like the display title toggle for blocks was introduced in this issue:

#1875260: Make the block title required and allow it to be hidden

As best as I can tell, we need to add something similar to this for blockForm:

    $form['label_display']['#default_value' ] => FALSE,

@mdrummond re: #83 That looks like the right answer but it won't work because the configuration form is unioning™ the blockForm() onto it's array so it won't override BlockBase::buildConfigurationForm. But I do believe you can change it through $this->configuration['label_display'] Though I'm not sure where to put it best... constructor would be my guess.

Tested the suggestion in #84. It works, and seems like the right place to put defaults to me.

Side note: Bartik is currently hiding all block titles in the header region via CSS.

Two things in this patch and interdiff:

1) Changed the site__ classes to site- classes in the twig template.

2) Toggled the block title display to off by default. After a lot of tries, this was surprisingly easy. Just needed to add 'label_display' => FALSE to the defaultConfiguration array. The other magic that makes that work is inherited from BlockBase, at least as far as I can tell. Hopefully this is the right approach. It does seem to work fine, from several tests I did.

Go go green testbot!

Would be good to get some extra eyes on this patch to make sure that this approach looks doable. If so, next items on the to-do list are:

1. Create branding region in Bartik. (Is this needed in Seven, which doesn't have branding? Stark?)
2. Load site branding block into branding region on install.
3. Ensure that branding region and branding block have same CSS as previous branding elements.
4. Remove site branding elements from page template in Bartik (check if removal needed in Seven and Stark too).
5. Change appearance page and config to remove ability to turn site branding elements on and off.
6. Make any necessary changes to tests. If possible name the new region branding rather than the current name-and-slogan id.

Any thoughts on if those should be a separate follow-up issue to this one, or if we need a mega-patch that does all of those things?

Personally, I'd love to start by just getting the ability to add site branding elements anywhere on the site. I'd hate to see that ability get significantly delayed by the need to get all of the above items completed first.

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,137 @@
   +use Drupal\block\Annotation\Block;
   +use Drupal\Core\Annotation\Translation;
   

   These aren't necessary.

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,137 @@
   +use Drupal\Core\Config\ConfigFactory;
   

   Typehint with the interface whenever possible; change this to Drupal\Core\Config\ConfigFactoryInterface here and in corresponding places below.

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,137 @@
   +   * @param \Drupal\Core\Config\ConfigFactory $configFactory
   +   *   The factory for configuration objects.
   +   */
   +  public function __construct(array $configuration, $plugin_id, array $plugin_definition, ConfigFactory $configFactory) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +    $this->configFactory = $configFactory;
   +  }
   +
   

   Object properties are lowerCamelCase, but variables are lower_snake_case, I believe: so $this->configFactory = $config_factory;

4. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,137 @@
   +    $build['#site_slogan'] = XSS::filterAdmin($site_config->get('slogan'));
   

   "Xss" is the actual class name. (PHP doesn't care about class name casing, but we should!)

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,137 @@
   +   * Overrides \Drupal\block\BlockBase::blockForm().
   

   "Implements" and "Overrides" docblocks that don't add any more documentation should just be {@inheritdoc} instead:
   /**
   * {@inheritdoc}
   */

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,137 @@
   +  /**
   +   * Overrides \Drupal\block\BlockBase::blockSubmit().
   +   */
   

   @{inheritdoc}

Here's a patch that addresses the concerns in #88 and #89.

Tried it out, seems to still work fine.

#87 I'd like to see this land, then follow up:

• 1, 2, 3, 4 doable here #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo)
• 5 plausibly another patch, with 6 being as required.

Agree with no more delays, we need to agree on an approach which plausibly gives us a blueprint for the other elements-as-blocks patches also, to me this is looking very good for users and themers, being able to lock down that source order is a big win, negating much hijinks required in CSS.

I can start working on items 1-4 later today, starting with the work done in #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo). In the meantime, it would be great to get some final looks at the patch in #90. If it's looking good, and somebody else wants to test this out, maybe this we can get this issue to RTBC?

Cloned D8, invoked patch, then went thru install. No problems. Created several 'site' blocks, moved them around, deleted a 'site' block, all with no problems. Think it's rtbc

We should actually try using this, if not in actual usage, then just in a test.

Additionally,

1. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,135 @@
   +  protected $config_factory;
   ...
   +    $this->configFactory = $config_factory;
   

   It should be protected $configFactory;
   The seeming mismatch of $this->configFactory = $config_factory; is correct.

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,135 @@
   +      '#title' => $this->t('Toggle branding elements'),
   +      '#description' => t('Choose which branding elements you want to show in this block instance.'),
   

   Half are using t(), the other half $this->t(). Please use $this->t() everywhere.

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,135 @@
   +    // Logo
   ...
   +    // Site name
   ...
   +    // Site slogan
   

   Either make these full sentences, or just remove them.

I'll take care of making those three changes tonight. I haven't written tests before so I might need to find some assistance with that.

The other thing I was thinking about was that we may want to get the markup/css to match more closely with the pattern in system's page.html.twig. One big thing is to change the wrapper for the site name to strong instead of h1. We don't need h1's all over the page.

When we do the region setup, we could possibly add a feature to the block so that if the block was in the branding region, and if the page didn't already have another title with an h1, then change the wrapper for the site name to h1 rather than strong.

If a theme wants to have the main branding for the page in a region other than branding, with an h1 for the site name, then that could be handled within that's themes templates.

One other discrepancy I noticed was that by default, the branding elements use class names, while in Bartik the branding elements use id attributes for styling hooks. For the generic block elements, we should probably use class names, since there could be multiple copies of the block. For Bartik maybe there's a way to put an override on the branding twig file to use id attributes if the block is within the branding region?

I'm not sure why Bartik uses id attributes for styling hooks rather than class names but changing that is probably beyond the scope of these issues, I'd assume.

This patch addresses the suggestions Tim made in #94.

I also changed the markup to match the pattern in system's page.html.twig file. That should make it easier to ensure other blocks behave the same as the default and will make the branding region issue a bit easier.

I also tweaked the logic in the twig file and clarified the docblock. The docblock now explains that the use_ variables come from the block settings, not the appearance page. Eventually we'll probably want to remove the toggles for site name, site logo and site slogan from the appearance page, but that's a separate issue.

The site name is required, but the site logo and site slogan are not. While there is a default site logo of the Druplicon, there is no default site slogan. So this now tests both if the block asks to use the slogan and if a slogan actually exists. page.html.twig just tests if the site name, site logo and site slogan exist for those parts of the markup.

We could test if the site name exists, but since it's a required field, I'd be hard pressed to think of a situation where that wouldn't be set.

It might be unlikely for a site logo to not exist, but it is possible, so makes sense to test for that I think.

Go testbot go!

Oh one more note on the tests. In talking to Joel, my understanding is that to test if the site branding block works we would need to have an instance of the block be created in a region by default. Since the plan is to create the branding region in a separate issue, it might be hard to set up the tests on these site branding blocks right now. However, I don't know a lot about tests, so I could be completely wrong on that.

Would some additional manual testing be sufficient to commit this?

re: tests: that's the only way I can think of testing this. Not a testing expert by any means(copy paste and manipulate existing tests).

I'm not a testing expert either, but I don't think we need to test that the block displays in a particular region. The whole point of this patch is that we don't want to enforce the location of the branding elements.

I think we do need to test that the block settings for showing/hiding each branding component work as expected, though. Here's that.

The site name off test will likely fail, as we don't have an if for an empty site name since that is required.

We should also probably test the use_site_name, use_site_slogan and use_site_logo settings which are different. You might also want to test site_name exists but use_site_slogan off to see if the name-and-slogan exists.

There are probably a ton of combinations for each of these features too.

This actually is testing use_site_logo, etc. The configuration key for that in the patch is 'site_logo'; you're just representing that at build time as 'use_site_logo'.

it might be a good idea to tweak the patch so that the configuration options are actually named 'use_site_*' instead of just 'site_*'. Less ambiguous that way.

+++ b/core/modules/system/templates/branding.html.twig
@@ -0,0 +1,40 @@
+{% if use_site_logo and site_logo %}

I haven't reviewed the rest of the patch but this seems a bit awkward, is it really necessary in the template (slogan also)?

Les: That's probably a good idea to switch the config options to use_site. Definitely more accurate that way. I can update that tonight.

Jeff: Yes, I believe that's necessary. We have two possible situations. One, somebody could use the block setting to not display the site slogan. Two, somebody could not set a site slogan on the site information page. We should only show the slogan if the block is set to display the slogan and there is actually a slogan to display.

1. +++ b/core/modules/system/templates/branding.html.twig
   @@ -0,0 +1,40 @@
   +{% if use_site_logo or (use_site_slogan and site_slogan) %}
   +
   +    <div class="name-and-slogan">
   

   We should outdent the div.name-and-slogan by 2 spaces, that should fix the indentation here. I'm also not sure why this condition needs to care about use_site_logo, seems like that should be use_site_name.

2. +++ b/core/modules/system/templates/branding.html.twig
   @@ -0,0 +1,40 @@
   +    </div>{# /.name-and-slogan #}
   

   I know we have some of these in core already but that was just keeping them in the process of converting from .tpl.php. I don't think we should add any new ones because there's no standard saying we should and most folks I've talked to are against them in the first place.

{% if use_site_logo or (use_site_slogan and site_slogan) %}

That is a typo on my part. Indeed, it should be use_site_name.

I'll take care of the outdenting and comment removal tonight.

Couldn't we do something like this?

if($this->configuration['site_logo']) {
  $build['#site_logo'] = theme_get_setting('logo');
}

if($this->configuration['site_name']) {
  $build['#site_name'] = $site_config->get('name');
}

if($this->configuration['site_slogan']) {
  $build['#site_slogan'] = Xss::filterAdmin($site_config->get('slogan'));
}

I tried but it always broke...

Also, I think the name-and-slogan div makes no sense when we can place the names and slogans in separate blocks using multiple instances. I could have two name-and-slogan divs on the page where one only includes the name and one only includes the slogan. Do we even need a div to wrap both?

re: #107, I just had a chat with Cottser about this very thing:

9:57 Les_Lim instead of "use_site_slogan" in the twig template, is it more appropriate to do the conditional logic in the build() method?
9:57 Les_Lim or should the twig template know what the block settings were?
9:57 Cottser Les_Lim: could go either way. I think having display logic in Twig templates is perfectly fine
9:57 Cottser and I don't think we should be scared of logic in templates
9:58 Les_Lim k
9:58 Cottser probably better to expose that to people theming this stuff I'm thinking.
9:59 Cottser Les_Lim: more control via the templates that way. you could make the template ignore the UI settings if that's the way you feel :)
9:59 Cottser but I could understand doing it either way.

I'm fairly convinced by that; we shouldn't make judgments about what information the template doesn't need. There could very well be legitimate reasons for the template to have the logo or slogan available, even if it's set not to display.

Lewis:

The name-and-slogan div is what's currently in the system page template and in Bartik. I'm just trying to match what already exists so that we stay consistent. Those existing templates have the same logic: there is a name-and-slogan wrapper, even if there is only a name or a slogan.

I assume it is set up that way to allow for consistent styling for themers. In Bartik the name and slogan float to the right of the logo: I'd have to spin up a test site, but I'd bet a dollar that the floating is done on the name-and-slogan div rather than on the name or slogan elements, and that's why this markup pattern exists.

Getting rid of the wrapper div might not be a bad idea, but I'd suggest doing that as a separate issue once we get the ability to add branding blocks and then complete the next issue, which is replacing the current branding markup with a branding region and default branding block.

I could go either way on having the display logic in the build step or the template, but giving more options to themers in a twig template rather than an object oriented plugin is probably a good thing.

@LewisNyman the use_* prefix is there to represent the checkbox boolean value. It has no representation of if those values exist or not. I'd prefer to keep a one to one mapping of those variables. The name could change to is_* if that is more consistent with other booleans, but I thought use_* would be more appropriate. You may need those variables like sitename/slogan for page titles and other areas on the site, so the checkboxes seem like a good approach and place to put them on the block that is using them.

I'd have to agree with @Les lim

There could very well be legitimate reasons for the template to have the logo or slogan available, even if it's set not to display.

.

We don't necessarily even need to check that the image, site name or slogan exist to display their markup, that may be too much hand holding and people could just look at the markup to realize "Duh!, I forgot to add the logo, that why the src="" is blank." @mdrummond just added those in there to try to map close to what we have already to ease this patch into committal.

Re #109 I tend to think that if we're doing multiple issues anyway to solve this in full we can add a template override in Bartik later if we need/want to preserve the name-and-slogan wrapper pattern in Bartik. IMO it would be nice if this block's markup was more generic and didn't make assumptions about patterns or use cases.

I'll have to check but I'm pretty sure the name-and-slogan pattern is in system as well as Bartik.

You're right, so changing that would probably be a dream markup thing then if anything (#1980004: [meta] Creating Dream Markup). Carry on :)

Here's a patch to address all of the above issues.

1) Renames the config to use_site_name, etc. which should be clearer. The tests have been adjusted to get the settings for these config names.
2) Fixes the outdenting issue and the typo in branding.html.twig, as well as removing the comment on the closing div.
3) Some cleanup work in the tests to use similar naming conventions as in the rest of the patch. I also added test coverage to see if the name-and-slogan div is properly turned on and off based on the settings for using the site name and site slogan.

Hopefully this addresses the remaining issues and testbot goes green. Then on to manual testing!

Can we just pretend I didn't end the interdiff file name with .patch?

Thanks.

I was pretending till you brought it up:P

...we should really get used to hiding previous, obsolete patches/files when uploading new ones ;)

Anybody up for manually testing this?

I did some manual testing and everything checks out as far as functionality. Here's some nitpickery and docs suggestions, I love this patch!

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
   @@ -0,0 +1,116 @@
   + * Definition of Drupal\block\Tests\BlockSystemBrandingTest.
   

   This should be "Contains \Drupal\block…", Definition of without the leading backslash is an outdated standard.

2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
   @@ -0,0 +1,116 @@
   +    $this->assertTrue(empty($site_name_element), 'The branding block site name was found.');
   +    $this->assertTrue(empty($site_slogan_element), 'The branding block slogan was disabled.');
   +
   +  }
   

   Super minor but we can lose the blank line at the end of this test method.

3. +++ b/core/modules/system/templates/branding.html.twig
   @@ -0,0 +1,40 @@
   + * - site_logo: Logo for site.
   + * - site_name: Name for site.
   + * - site_slogan: Slogan for site.
   

   These might read better (or less terse) if they were just "Site logo.", but it also might make sense to indicate where these variables are configured. (Site information/theme settings).

4. +++ b/core/modules/system/templates/branding.html.twig
   @@ -0,0 +1,40 @@
   +{% if use_site_logo and site_logo %}
   +  <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo">
   +    <img src="{{ site_logo.url }}" alt="{{ 'Home'|t }}" />
   +  </a>
   +{% endif %}
   +
   +{% if use_site_name or (use_site_slogan and site_slogan) %}
   +
   +  <div class="name-and-slogan">
   +
   +    {% if use_site_name %}
   

   Why does the name and slogan one have all the extra newlines? Seems like we don't need the newlines when we have indents. Just have a newline to separate the logo from the name and slogan.

What Cottser said, and:

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
@@ -0,0 +1,116 @@
+    $this->assertTrue(empty($site_name_slogan_element), 'The branding block name and slogan div was found.');
+    $this->assertTrue(empty($site_name_element), 'The branding block site name was found.');

The message doesn't match the condition in these tests; both of these lines are testing that those elements are disabled.

Here's a patch to fix the issues in #122 and #123. Thanks for reviewing this!

Green! One step closer!

May be nice to have field help text to tell users where to set those variables for logo and site name/slogan?

Here is a patch to add that help text. Also removed a blank line in the twig docblock between the two types of variables.

Minor things from reading through, otherwise I think this is RTBC.

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
   @@ -0,0 +1,115 @@
   + * Tests site branding block.
   

   Should probably read something like:
   * Provides testing for the branding block module functionality.

2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
   @@ -0,0 +1,115 @@
   +    $site_name_slogan_element = $this->xpath('//div[@id="block-site-branding"]//div[@class="name-and-slogan"]');
   ...
   +    $this->assertTrue(!empty($site_name_slogan_element), 'The branding block name and slogan div was found.');
   

   Testing that the wrapper div is there doesn't do us any favours, probably can be scrapped in all instances.

This has:

• Adequate test coverage.
• Been manually tested by a number of people.
• Allows us to simplify our page.html.twig in the follow-up.
• Provides much more flexibility for contrib to selectively hide and place branding elements within different layouts.
• Keeps the markup inline with what is currently used in core

Feel free to disagree, if I missed anything.

1. +++ b/core/modules/system/templates/branding.html.twig
   @@ -0,0 +1,35 @@
   +{% endif %}
   

   Unnecessary new line?

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,135 @@
   +      '#description' => $this->t('Defined in appearance or theme settings.'),
   

   Capitlize Appearance? I.e. Defined in Appearance or theme settings.

3. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,135 @@
   +      '#description' => $this->t('Defined in site information settings.'),
   

   Capitalize Site? Same with Slogan description, i.e. Defined in Site information settings.

This addresses the concerns in #131. Changed the capitalization in branding.html.twig as well.

Would be great for somebody to look this over quick to get back to RTBC.

Passed if anybody wants to look this over again.

Thanks @Jeff Burnz, I'll let you re-RTBC if that fits the bill. Thanks for the space catch, I should've noticed.

Thanks @Jeff Burnz, I'll let you re-RTBC if that fits the bill. Thanks for the space catch, I should've noticed.

Cottser provided some additional feedback in IRC. Changed some comments and also refactored xpaths for branding elements into variables in case those need to change in the future.

Oops, 'sprint' is the right tag for http://drupaltwig.org/issues/focus :)

Go @mdrummond go! I think this is looking very very good.

Looks good to me, those were very minor niggles.

I've spent quite some time theming with this patch and I find it hard to fault. I think some novice themers might get stumped by the template instance issue - meaning you have to theme at the block instance suggestion level to modify the markup per instance, however this conceptually little different to breaking out $content in a node template, just different template names. I mention this here as I think we should put some help along those lines into the change notice.

All in all I really like it, as a themer I can definitely work with this and I think its a nice win for site builders.

FWIW, turn twig_debug on in settings.php and you will see the template suggestions for each block instance in HTML comments :)

#138, yes thats a good tip in general.

I'll clarify #137 for reviewers that don't fully understand what I mean, this is the sort of thing I'm alluding to for the change notice.

The root template is branding.html.twig, however there is no such thing as branding--2.html.twig. For example if you have two instances of the branding block it might be somewhat intuitive for a themer to think that there would be a branding--2.html.twig type template suggestion for instances - there is not.

Additionally there is no way (as it stands) to provide this suggestion via hook_theme_suggestions_branding_alter(), instead the way to theme is to ignore branding.twig.html and use the per block instance suggestion, aka:

• block--sitebranding.html.twig
• block--sitebranding-2.html.twig

Here you will print variables from the block $content array, for example lets print the logo in block--sitebranding-2.html.twig, because we want to remove a wrapper or whatever:

<div{{ attributes }}>
  <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo">
    <img src="{{ content['#site_logo']['url'] }}" alt="{{ 'Home'|t }}" />
  </a>
</div>

While this is intuitive and easily grasped by those of us who work with drupal 8 all the time, for new Drupal themers this might be a bit of a wtf moment so having some docs lying around that state it strait out could be useful.

@Jeff Burnz - Great point, I missed what you were getting at with overriding branding vs overriding the block template.

In that case we may want to pass an array of theme hooks to the render array, assuming we have more information about the block instance in the build method of the block plugin.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
@@ -0,0 +1,135 @@
+    $build['#theme'] = 'branding';

Could be $build['#theme'] = array('branding__' . $some_kind_of_id, 'branding');

That doesn't necessarily need to happen here, could be a non-major follow-up issue.

Awesome!! I am so happy to see work being done on this! :)

ATM it doesn't seem like there's a change record required for this, since this just adds an optional block... but I was a bit thrown by Jeff's comment in #139... but I think this is true for all block instances, no?

Things that I noticed while testing:

1. The block looks funny when actually used (left is built-in site branding vs. right is the new block):

   

   ...but I guess fixing this has been split off to #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo) instead. I'm really curious how we plan to solve that, since with these branding elements being blocks, they can now go into literally any theme region.

2. One thing we are missing is some links here to the proper admin pages to set these things, at least for people with access to them:

   

   We leave people up a creek otherwise since they're not going to know where "site information" settings are necessarily (they set them on install, and typically don't have a reason to go back and change them after).

3. Ideally, you wouldn't need to jump around at all, and could just set those settings right here in-line. I'm guessing that wasn't done here once again to minimize the impact of this patch on the code base. (And needs some thought, since site name has implications elsewhere than just this one block.)
4. This interface also gets into some "fun" UX problems. For example, if "Site logo" here is checked, but "Logo" visibility is toggled off in admin/appearance/settings, no logo will appear in the block despite the value of this checkbox being true. And worse, if you only have "administer blocks" perms but not also "administer themes" perms (which seems like a reasonable set-up for many site configurations) the block will just appear to be completely broken for you. I would've expected these checkboxes to be informed based on those global settings (or, once again, remove them in favour of this instead).

Given it took us 3 years to get to this point, I'm a bit torn. On the one hand, I'd love to commit this to get a stake in the ground and make some progress on this effort. On the other, though, I'm pretty worried that if we don't take on a pretty hefty chunk of work, which includes at least:

- Make sure this block looks decent and can actually replace the built-in site branding in Bartik.
- Remove the old global theme settings and use the block configuration instead.
- Probably? move the site name/slogan/etc. fields to the block itself vs. on some random disconnected admin page.

...then we leave 8.x in a bit of an inconsistent state with this block that creates more confusion than it helps solve.

So I lean towards committing it, with a "revisit before release" tag so if that follow-up work doesn't get done, we could always revert this patch (that's a big advantage of the "split off" approach that was chosen here). But before doing that, I'd love to hear from the drivers of this patch on what they view as the next steps to get this "shippable" and how much work that looks like.

I'm not clear if the issue raised in #139 is specific to this patch, although given Cottser's suggested fix in #140, I think that's possible. Cottser seems to indicate this could be addressed in a follow-up issue, but if you think it needs to be handled here, we can do so.

Here are my thoughts on the other issues you raised:

1) The approach I was taking in the branding region follow-up was to ensure that the branding block looked the same when in the same location as it previously was (the newly minted header_first region). I'm not sure that we can or should ensure that the branding block looks the same in every other region. For example, a branding block in a sidebar probably may or may not have enough room to have a longish site name and slogan floated to the right. We could set up some component-based styles that would work in any region, but we'd have to define what styles are important enough for that. Those component-based styles could be defined here or in the branding region issue.

2) Making a link to the site information page, where the site name and site slogan is defined, is easy. The logo, however, can be defined either on the appearance page, or on the theme settings page, which overrides the appearance page setting. A link to the appearance page is simple. A link to the theme settings page gets more complicated.

A block can be used in any enabled theme. So this block could be administered from the block layout page for any theme. Thus a link to the theme settings page would need to know which theme tab on the block layout page somebody was on before they came to this block configuration page. That seems... complicated.

Because of that, we just gave general directions on where to find the settings where these variables are defined.

3) The site name, slogan and logo are global settings that can be used throughout the site. There could in theory be 12 branding blocks on the site. I think having each one define the site name, slogan and logo could get very, very confusing.

4) The plan (at least my plan) is to remove the show/hide toggles for the site name, logo and slogan from the appearance and theme settings page. We were planning to do that in the follow-up issue, since that is where the branding elements will be removed from the page templates for system/Stark and Bartik. At that point those elements will only appear through the block, so those toggles won't make any sense anymore and should be removed. The branding blocks created by this patch actually will not be controlled by those show/hide toggles (at least I think so, based on how this is set up).

My plan on what we'll tackle in the follow-up issue is:

1) Split header region in Bartik (and system/Stark?) into header_first and header_second

2) Install puts a branding block with default settings into header_first

3) Ensure that branding block in header_first region has same CSS as previous branding elements

4) Consider what CSS for branding elements should be component-based, so that it can follow the branding elements to any region.

5) Remove site branding elements from page template in Bartik and system/Stark

6) Change appearance page and theme settings page to remove ability to turn site branding elements on and off.

7) Clean up a bit of the HTML/CSS in the branding block. The name-and-slogan wrapper is kind of weird if there is only a name or only a slogan. Probably there to float the name and/or slogan to the right of the logo. Look if there's another way to do that.

8) Add the ability to target specific instances of a block, as mentioned in #139/140.

Items 1, 2, 3 and 5 are already done in the follow-up patch, at least for Bartik. A little more work needs to be done to get those changes made to system's page template too.

In short, I think it's very doable that we can tackle those problems.

I don't agree that the site name, logo and slogan should be set in the block. That seems like a completely different issue if we wanted to consider it at all.

As for the links to the site information and appearance page. That's easy. It's the theme setting page that's a doozy, and I don't have a good answer for that, which is why I left it off. Personally I don't think that's a deal breaker. If somebody knows enough to be able to manage a block, they probably know enough to navigate to a theme settings page on their own.

Kid's crying, so I gotta go. Interested in your thoughts on the above.

Awesome, thanks for the detailed response!

That's a really good point that there might be multiples of this branding block, and the global settings would apply to all of them (I also basically talked myself out of this when I thought about various instances where "Site name" is used: e-mails, etc.). That's a good case for leaving things where they are and just linking to the appropriate admin pages. So forget that terrible idea. :)

Also a good point that linking to the theme is a bit tricky, although it shouldn't be *too* hard. You got to the that settings form in the first place because you clicked "Place block" (or "Configure block") from the block layout page, which indicates which theme it's placing blocks on, so presumably the theme name is somewhere accessible from here.

Thanks a lot for the detailed next steps. Out of those, the ones that feel like they'd be nice to solve in this issue prior to commit are:

- Linking to the correct admin pages from the block config form (I realize this seems pedantic, but without it I worry we're introducing a UX regression if it never gets cleaned up)
- Cleaning up the "name-and-slogan" part of the template
- Adding a means so that the block is targetable by themers (unless this is an issue for all blocks, not just this one?)

The reason I say this is because this block is the first out of X issues that will add a block to replace hard-coded theme variables. As such, it'd be nice if it established good patterns we could then replicate on all of the other patches and make review of those lickety-split. It also cuts down on the number of follow-ups to track, since there are already at least 5-6 of them you've identified.

Not marking down from RTBC though, in case there's push back on that proposal.

So from the block layout page, if you click to add a site branding block, the path is:

admin/structure/block/add/system_branding_block/bartik

I would imagine we might be able to test the URL args to nab that last bit to get the referring theme and thus a link to the correct theme settings page.

You can create a branding block using the above URL without the theme name at the end. So if the theme name isn't in the URL, maybe drop the link to the theme settings page, particularly because you can then choose to put that block into different regions for different themes.

Does that plan sound ok?

I'm looking at the name-and-slogan part of this, and this is a bit tricky in terms of the appearance plays out in Bartik.

In Bartik, the logo is floated to the left, and then site-and-name is also floated to the left. That allows the slogan to appear under the name no matter the size of the logo.

If we removed the wrapper div, it gets a bit ugly. You can't just float the site name and the the site slogan to the left, because then the slogan will likely appear to the side of the name.

If the logo were always a certain size, then you could not float the logo, put the name and slogan ahead of the logo, float them to the left and apply a margin-left the same size as the logo. But the logo size will vary widely, and with a very thin logo, you could still get the slogan appearing below the logo.

So... I'd suggest that what we want to do is not remove the wrapper around the name and slogan but instead change its class name. I would suggest site-branding-text as a possible class name.

In theory we could drop the wrapper div from Stark, although if we did I'd suggest putting a wrapper div or p around the site-name's strong element: otherwise the name will appear to the right of the logo, while the slogan appears below the two of them.

I think changing the class name for the name and slogan wrapper to site-branding-text or something similar to that is probably the neatest solution.

Thoughts?

One other issue:

For the links to the site information page, appearance page and theme settings page, do we need to check access permissions before displaying the link?

I'm not sure if there's a proper D8 way to do that or to check the arg value that might or might not contain the theme machine name. Hopefully I can catch somebody in IRC to walk me through that.

Ufda.

So, I've done a number of things in this patch.

1) Removed the name-and-slogan wrapper in system/Stark. I also changed strong class=site-name to div class=site-name. This is a visual change, but it allows the site name to stand on its own rather than appearing to the right of the logo. As long as we're making changes, I wouldn't mind if the divs for site-name and site-slogan were p elements instead, but we can leave that for a Dream Markup issue so we don't get into too many quibbles.

2) Added branding.html.twig to Bartik, so that Bartik can have an equivalent of name-and-slogan, which I titled site-branding-text. This allows the name and slogan to float to the right of the logo, but doesn't have such an odd name when only the name or slogan is present.

3) Updated the CSS in Bartik to account for these markup changes. Please note that since we haven't yet removed the markup in Bartik's page.html.twig, the old id-based selectors still need to stay in. Those will get pulled out later. Also note that while this makes the appearance of branding blocks roughly similar in other regions it may not be identical, because some regions change the font size or do goofy things like put a white border around the region (I'm looking at you, header region). I don't think it's our responsibility to override those regional settings for the sake of the branding block. That would be a nightmare.

4) Added the requested links to the configuration pages. That meant doing dependency injection to get an url injector as well as the request module.Yay Symfony! I also had to alter BlockFormController (per Tim Plunkett's suggestion) in order to make the block aware of its own theme in case the theme isn't available in the request information. This also checks to see if the appropriate permissions are available for the appearance/theme settings pages and the site information page. If the permissions aren't available, the block form even provides a helpful little message explaining that.

The one thing this patch doesn't do is allow block instances to get their own twig file. To be fair, however, I can't find a single block in core that does allow for this. I checked with larowlan, and he explained that a block plugin can't know about a block entity for caching reasons. He suggested hook_block_view_alter as an alternative. Or possibly hook_block_view_BASE_BLOCK_ID_alter. I'm not clear on exactly how to bring in a block instance with that, maybe somebody else has some ideas.

So anyhow, that's my evening's work. Seemed to work as I was trying things out. Time for everybody's favorite singalong: "What Does Testbot Say?" (#2203045: What Does Testbot Say?).

Good night and good luck.

1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -93,6 +93,16 @@ public static function create(ContainerInterface $container) {
   +    if ($entity->get('theme')) {
   +      $theme =  $entity->get('theme');
   +    }
   +    else {
   +      $theme = $this->configFactory->get('system.theme')->get('default');
   +    }
   

   I would rewrite this as

   if (!$theme = $entity->get('theme')) {
     $theme = $this->configFactory->get('system.theme')->get('default');
   }

2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -93,6 +93,16 @@ public static function create(ContainerInterface $container) {
   +    $form_state['block_theme'] = $theme;
   
   @@ -254,6 +263,8 @@ public function form(array $form, array &$form_state) {
   +    $form_state['block_theme'] = $theme;
   

   Why twice?

3. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
   @@ -0,0 +1,114 @@
   +  public static function getInfo() {
   ...
   +  function setUp() {
   

   These should have @inheritdoc, and setUp should be protected

4. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockSystemBrandingTest.php
   @@ -0,0 +1,114 @@
   +  function testSystemBrandingSettings() {
   

   should be public

5. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,210 @@
   +  public function url($route_name, $route_parameters = array(), $options = array()) {
   +    return $this->urlGenerator->generateFromRoute($route_name, $route_parameters, $options);
   +  }
   

   This should definitely not be public. Also, do we even need the helper function? Might as well just use $this->urlGenerator in each place.

6. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,210 @@
   +    // Get the theme.
   +    if ($this->request->attributes->get('theme')) {
   +      // Get the theme from request if available.
   +      $theme = $this->request->attributes->get('theme');
   +    }
   +    else {
   +      $theme = $form_state['block_theme'];
   +    }
   

   I think you should skip checking the request, and just rely on the form_state. Then you can ditch injecting it completely.

7. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,210 @@
   +    $administer_themes_access = \Drupal::currentUser()->hasPermission('administer themes');
   +    $administer_site_configuration_access = \Drupal::currentUser()->hasPermission('administer site configuration');
   

   The user should be injected.

8. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemBrandingBlock.php
   @@ -0,0 +1,210 @@
   +    } else {
   ...
   +    } else {
   

   Two lines

Went through all the changes Tim suggested in #148 and made them all.

We also spent a good amount of time working through how one might be able to target the instance of a branding twig file.

The short version is that this might not be possible at present. If we were dealing with a block twig file, there's already a block suggestions hook to take care of that. But since this isn't actually a block, it's the content inside of a block, it's too deep to be able to get the entity_id for the suggestion basically.

I'm guessing the alternatives are to maybe rewrite the twig file so that it's a block in all its glory or maybe to create a new suggestions hook to handle twig files like this. It seems a bit heavy to put all the regular block code inside of the twig file, so I'm guessing this may be a larger issue that needs to be solved elsewhere.

Hoping this goes green, all the problems are magically solved, and we can give the thumbs up. I'm on the v12 branch for this patch, but who's counting?

Too late for me to check this tonight but block.module declares it's own suggestions for block templates, so maybe system.module needs to do the same for branding, if that's even possible?

I should clarify #139, I'm not really saying we should have suggestions. What I actually meant is, if possible, to provide a way for themers to add suggestions - if they wanted to. Right now this is totally themable as it is, #139 is/was a feature request, I wouldn''t let this hold up the issue at all. I'll review this later today also, right now punished for time.

Right now, system.module has the following suggestions:

• system_theme_suggestions_html
• system_theme_suggestions_page
• system_theme_suggestions_maintenance_page
• system_theme_suggestions_region

That covers some of the templates for system, but not the other 18, including branding.

The question would be what information would be available to system_theme_suggestions_branding in $variables['elements'].

block_theme_suggestions_block uses:

• $variables['elements']['#configuration']['module']
• $variables['elements']['#plugin_id']
• $variables['elements']['#block']->id()

If similar information was available to us, I would imagine we could set up a suggestion hook similar to block's allowing robust theming of this. I have to run to work, so I can't test this right now, but if anybody has thought on where some of that info would be available in a branding hook, that would be helpful. When I was doing testing last night, it was the entity ID that had the right hook for a branding instance.

I do think that it's a good goal to make instances of this themable. I think it's much, much easier to explain to themers that a theme's twig files can override core's twig files or an instance of a twig file, rather than saying some can have instances and others can't. Maybe Twig debug provides that info as to what suggestions are available?

Anyhow, patch above went green, so hopefully we're getting really, really close.

I don't quite understand the need for a separate branding.html.twig template.
I think that custom block--system_branding_block.html.twig templates can be used, there isn't *that* much extra wrapping it.

If you thought, hey, the 13th branch for this patch is likely going to be the lucky one, let me inform you: you are wrong.

This patch does not work, but hopefully it's a step in the right direction, in that with a few pointers, this can work, or we decide this approach is unworkable.

Per a discussion with Cottser and Webchick on IRC, this patch tries to do the following:

1) Converts branding.html.twig from a template for the contents inside of a block to the entire block itself. But wait, there's more!

2) Creates a block within a block to extend the block contents. This Twiggy goodness means that—in theory—the block wrapper HTML can still be controlled from block.html.twig while this template just reworks the content based on the available variables.

3) ???

4) Profit.

The problem is that the block contents don't show up, which seems marginally problematic. The suggestions function in block.module doesn't seem to like the block ID, nor does it like the elements in general.

I'm a bit nervous that #1989568: Remove block config ID from being used as an HTML ID or template suggestion might render the whole issue moot. Without a config id, I'm not sure how you get the block instance. Entirely possible I'm not understanding, though.

You're welcome to tell me the things I am doing wrong in this patch. There are probably a good deal of them.

That linked issue would indeed make this override dead in the water.

But the OP asks a question:

What are use-cases where an HTML ID is needed or where you'd want to implement a template override on a per-instance basis (i.e., you added two blocks with the same exact configuration, but different machine names, and you want their markup to be different)?

This is the first time anyone has come up with a real answer to that.
The other answer is that the "branding block" is doing triple duty, and is overkill.

@mdrummond - I want to thank you for your continued work on this issue. Please don't get discouraged, issues often change direction and in my experience having 13+ versions of a patch is not necessarily unusual :) we're all collaborating here towards a good solution.

Here's a rough proof of concept patch based on what was discussed in IRC yesterday. This is relying on a pre-existing theme suggestion provided by block.module.

Please ignore the set tags and render_var stuff in the Twig templates for now. That is just an ugly workaround for #953034: [meta] Themes improperly check renderable arrays when determining visibility.

The most drastic change from the previous patch is that what you configure in the block settings determines what variables are available in the template. With this render array + block template approach, checking all the block toggles in the template would be really ugly, see #2160611: Provide {{ item.item.alt }} Twig syntax for getting data from $item['#item']['alt']. Here's a sampling of what it would look like without the previously-mentioned issue:

{% if content['#use_site_name'] or (content['#use_site_slogan'] and slogan) %}

If there's a use case for having the template disobey the block settings, that's something that would still be possible to achieve via preprocess. The visibility toggles are something I really don't think needs to be available in this template out of the box but maybe I'm missing something.

I think the fails in the last 2 patches are whitespace related, by the way. So nothing to worry about at least while we are trying to decide the best way forward.

I don't understand why we need to switch to render arrays rather than providing the variables directly to the template as we did previously.

What we had talked about yesterday was turning the branding template into a block template that contained the branding variables, then using extends to pull in the wrapper parts of the template.

That's what I tried to do in #155, although admittedly it wasn't working quite right. We should still be able to provide the variables directly to the the block--system-branding.html.twig template, right? And can't the Bartik template just override that just like any other Twig template?

I don't think that's possible. If you look at block_theme() it doesn't define 'variables' so we can't add to that when we define the block__system_branding_block theme hook. The block theme hook instead defines a 'render element'. Trying to switch to 'variables' for this specific instance will break things badly because preprocess functions, theme suggestions functions, etc. are based on 'render element', not 'variables'.

Edited x2 to clarify wording.

I'm going to ask a few questions that are probably dumb.

1) Is there a way for system.module to provide a hook extension of template_preprocess_block to add additional variables for the branding block?

2) It seems like the main thing we're getting from making this a full-fledged block is the suggestions function already in block.module. Couldn't we do a system_theme_suggestions_branding in system.module to create instance naming suggestions? I'm guessing the answer is that we wouldn't have access to the plugin_id or the config id? Seems like something that might be worth playing around with.

#161, this is just the difference between block_theme() using 'render element' instead of 'variables'. You can still put anything on the $variables['element'] you need.
#163.1, we want to avoid system module having block code.
#163.2, same answer. block_theme_suggestions_block() is dependent on code in template_preprocess_block(). A separate branding template cannot use any of that information, because it is two levels away: branding -> block plugin -> block entity

Now that I'm understanding this better, it makes a lot more sense.

I'll have to take a look at the visibility discussion above to see if there was a use case for providing the variable to templates even if the use is turned off. Using #access to determine visibility is pretty clever.

Would be nice to see #953034: [meta] Themes improperly check renderable arrays when determining visibility get in, but if it's not ready before this issue is set to go, we can create a follow-up issue to address that.

As per earlier discussions, we probably want the variables to be $site_logo, $site_slogan and $site_name.

Just so I'm clear, are we essentially putting the variables in via the build step rather than via system_theme?

I think we can probably get this ready and then have a follow-up for #953034: [meta] Themes improperly check renderable arrays when determining visibility.

This patch has doubled in size since #135. I'm not sure what the extra gets me. I know with #135 I can have a theme local copy of branding.html.twig where I can add more themeing handles if I want and modify the markup as well. True, there aren't many useful vars available but I can do some conditional stuff(e.g., I can change the output depending on whether I'm logged in or not). I thought the issue here was to implement the new block and have a separate issue to convert core themes to the new block. Am I really going to do very complex things with this block?

dcrocks:

By making the Twig template a full-fledged block, that allows for it to receive block suggestions for the template name. That means that in your theme, you can still have a local copy of this template to alter as you please. However, if you want to put a branding block in three different regions, and you want to adjust the markup a bit in one or two of them, you will be able to create a separate branding block template file to target a particular block instance.

That might not happen all that often, but this patch is setting a pattern for a number of other issues that are similar, for example menu blocks. You may want a different markup pattern for a menu block with drop-downs that would appear in the header, while you'd have a different markup pattern for a vertical menu that appears in your sidebar.

I think that's a good pattern to establish and is worth a little extra effort to get there.

You can still do all the customization you want with pretty much the same variables. If you want to make the site slogan conditional upon a user's role, you can do so.

The follow-up issue to this one will replace the default branding markup in Bartik and system/stark with a header region where a branding block can be placed on install.

FWIW I'm pretty much agnostic with regards to the approach taken here, I like both to be frank (pros/cons on both sides from my pov).

My question is how does this affect the other issues going forward - e.g. in breadcrumbs, status messages and the like? Right now those already have separate templates and was one reason I kind of like #135, it seemed more consistent with those approaches (thinking that status messages template would work like the branding.html.twig does in #135).

Like I said I'm not, at the moment, swayed either way, whatever is most consistent and easiest to understand is good (win for themers IMO), to be frank I found the template suggestion naming conventions imposed by #158 frightfully confusing.

That said, if there are compelling reasons (that I am blissfully unaware of) for doing #158 and we make this consistent for core, then right oh then, do it.

I hit one, possible unrelated but pertinent issue here - when I {{ dump() }} in the block you get a twig error:

Twig_Error_Syntax: A template that extends another one cannot have a body in "core/themes/stark/block--sitebranding-2.html.twig" at line 21. in Twig_Parser->filterBodyNodes() (line 370 of core/vendor/twig/twig/lib/Twig/Parser.php).

Here's a revised patch that uses site_logo instead of logo and site_slogan instead of slogan.

The only markup change in #158 was generating the img element within the build step rather than in the twig file. Is there a reason we need to do that rather than just passing the URL to the template and constructing the img element there? That makes it easier to add a class to the img if necessary.

Also, is there a reason the Twig file is named block--system-branding-block vs block--system-branding? I'm guessing we can't use block--branding either?

I read through #953034: [meta] Themes improperly check renderable arrays when determining visibility and geesh, the issue goes back 3.5 years with the Twig part at least 9 months. There's talk of a solution, but nothing concrete in terms of a patch that's close to being committed. So who knows how long it will take to fix that.

Setting and rendering the variables in the Twig template just to be able to properly test #access seems gross to me and probably will to other themers too, as Jeff indicates in #168. I'm also not thrilled with something like content['#use_site_name'].

Tim indicated in #164 that we can somehow still put things into $variables['element']. Any thoughts on how we do that? That would allow us to make use_site_name available in the template again without having to resort to content['#site_name'].

As for #168, I have no idea what the error about body in an extended template means. Strange.

In terms of how this would affect the other similar issues. If we take this approach for the others it means making full block templates with extends for each of those variable to template conversions. That's probably particularly important for the menu issue, as that seems to me the likeliest place that somebody will want to adjust markup in different block instances.

Hopefully we can get use_site_name back into the template and address the possible error in #168. Other outstanding issues are the block template name options and whether or not we can just pass the logo URL to the template and construct the img element there.

Error in the filenames.

To get it out of the way, the twig error brought up in #168 looks like it was due to putting dump() inside the "body" of the template extending block.html.twig. You can see the same error by putting {{ 'foo'|t }} outside of the {% block content %}…{% endblock %}. Because this is a template extending another template, the places you work in are the block tags, to optionally override the blocks defined in the parent template.

The template naming of block--system-branding-block is based entirely on existing block suggestions. That one in particular is based on the block's plugin ID set in the annotation:

 * @Block(
 *   id = "system_branding_block",
 *   admin_label = @Translation("Site branding")
 * )

I'm not sure what the naming conventions are for block IDs but to change that particular theme suggestion we do have the option of changing the plugin id. I'm guessing we'd want the system namespace at least, so maybe 'system_branding', but it seems like most other block plugins end in _block. The benefit of using block theme suggestions is that whatever core or contrib theme suggestions there are for a given branding block instance, they can be used. I used this suggestion because it should always be there unless someone forcibly removes it. If a module or theme hook_theme_suggestions_HOOK_alter()'d out this theme suggestion the branding elements will still render through the default block.html.twig because the block contents are just a render array. So the custom template is not really needed because we've built this out as a proper render array.

Regarding #953034: [meta] Themes improperly check renderable arrays when determining visibility we can always render the three parts of the render array via a preprocess function and drupal_render(), unfortunately that limits the amount of further manipulation possible but it would be only temporary because #953034: [meta] Themes improperly check renderable arrays when determining visibility needs to be solved before release (it's a critical issue).

Setting and rendering the variables in the Twig template just to be able to properly test #access seems gross to me and probably will to other themers too, as Jeff indicates in #168. I'm also not thrilled with something like content['#use_site_name'].

Tim indicated in #164 that we can somehow still put things into $variables['element']. Any thoughts on how we do that? That would allow us to make use_site_name available in the template again without having to resort to content['#site_name'].

I just want to note that we'd never ship a template in core with the set blocks and render_var stuff. It's only there as a proof of concept.

It looks like the block plugin's build method adds to $variables['elements']['content'] and I'm not sure we'd want to try and rewire that for our use case. The reason why we can't arbitrarily add variables to this render array is because it's a render array, so there are many points in the process where the array will go through things like element_children() and if you have $variables['elements']['content']['use_site_name'] = 1 you'll see an error like:

User error: "use_site_name" is an invalid render array key in Drupal\Core\Render\Element::children() (line 89 of core/lib/Drupal/Core/Render/Element.php).

Arbitrary data like this needs to be stored in a hash-prefixed #key in render arrays otherwise it's treated as a render element. The hash-prefixed keys are ugly in Twig templates when there are use cases for pulling out that arbitrary information and that's why I opened #2160611: Provide {{ item.item.alt }} Twig syntax for getting data from $item['#item']['alt'] a while back.

So if we want to push forward with this general direction of using a block suggestion, we should decide whether we want to use the #access and temporarily drupal_render() method, or whether we want to store the #use_site_name, #use_site_slogan, #use_site_logo information in the render array and pull that information out in the Twig template, even if the syntax is ugly at this point. Or maybe there's another way we haven't thought of yet.

I also wanted to mention that I've added this issue as an agenda item for the weekly Twig/theme system call that's happening in about 1 day + 7.5 hours at the time of writing. Anyone following this issue is welcome to join in and we can discuss how to move forward in real-time instead of composing novels to each other :)

http://everytimezone.com/#2014-02-27,720,6bj

If you join #drupal-twig on IRC we'll post the link there when the hangout starts, or you can keep an eye on @Cottser on Twitter, I'll tweet out the link to the hangout from there as well.

1) Keeping name as block--system-branding-block seems fine. If that works better for the suggestions system, cool.

2) If we can use a preprocess function for now to handle the drupal_render and access checks, maybe that would be a good way to go. If we're going to have a temporary solution until that issue is fixed, that at least allows us to keep the markup in the Twig file the same. Although now that I think about it, once we don't need the preprocess, we'd probably ultimately still need to change the markup, as then we'd be bring content['#use_site_name'] or ideally content.use_site_name back into the Twig file, right?

So I guess either way would require a change. Maybe it's better to put the set blocks and render_var into the template for now. If somebody pulls that into a theme now, it will still work after it's no longer necessary. However if we go the preprocessor route, then pull the preprocessing, then if somebody doesn't update their template there will no longer be an access check.

3) Only other question I have is if we can push the logo URL into the variable rather than the entire img, so that there's more flexibility with the img element markup in the template.

---

I may or may not be able to make the Twig call: I was only able to make last week's because I was working from home. If I make the call, it will likely be at the end. Unless I can figure out how to do the Hangout audio-only on my phone while I'm in the car during my commute. If this item could be at the end of the agenda, that would be great.

tl;dr
Re: #177 I'm going to push back too on the renderable arrays being passed in to the template. The #access trick is nice but the theming experience (FX as #mortendk calls it) has become a pain in the ass at that point. Would rather have the "if conditions" in the template then have to modify generated markup _alter hooks to modify an array to spit out an HTML tag with an extra class.

To address the last couple comments: