Problem/Motivation

The search form block uses the default block template, and thus needs to add classes and other attributes through preprocess. The main purpose is so that the Search button can be inline with the keywords input element.

Proposed resolution

Make Bartik and Classy templates specific to the search block that include the classes. Leave the preprocess function in Search that adds the "role" attribute, since this is specific to searching and should be done in this preprocess function.

Remaining tasks

Decide whether this should be done.

Pros:

  • Reduce the preprocess function in the Search module.
  • This is in keeping with the move away from using functions and doing everything in templates.
  • Easier for themers to control the class attributes printed in the search form block.

Cons:

  • Having to know this template is separate if you use Classy as your base theme... however since it is right there in the templates directory, it is probably obvious.

Original report by @Jeff Burnz

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category This is a task because it copies templates, makes some preprocess changes, but does not add new features.
Issue priority This is not critical because nothing is broken.
Unfrozen changes Unfrozen because it only changes templates and a preprocess function, and markup is not currently frozen.
Prioritized changes The main goal of this issue is improving themer experience (TX). This was a follow up to phase 1 Consensus Banana changes, removing CSS classes from preprocess, and can be considered part of those prioritized changes.
Disruption This is not a disruptive change, or at most a minor disruption which the benefits outweigh.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

Antti J. Salminen’s picture

Per the discussion in #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates. (comment #36 and on) this might end up not being part of these changes. May want to wait for the final word before working on this...

Jeff Burnz’s picture

I think we should close it for now, although perhaps Cottser might chime in here, but I don't think we should revisit things over and over etc, a decision was made and I am happy to live with that. Personally I would prefer practically everything came out of preprocess, it might be a pipe dream :)

davidhernandez’s picture

Title: Move search block classes from preprocess to templates » Add search form block Twig template file
Version: 8.1.x-dev » 8.0.x-dev
Component: theme system » search.module
Issue summary: View changes
Status: Active » Needs review
Issue tags: -banana, -frontendunited
Parent issue: #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates. »
FileSize
3.05 KB

Changing the issue, and uploading a patch. Please discuss.

If we get setAttribute() in, the role can be added that way. Technically, the attributes should be printed without role, because we could get a duplicate, but I don't imagine anything is going to try to set the role for this block.

jhodgdon’s picture

I don't have an opinion on whether we should or should not do this (not really involved in Twig or the parent discussion). But I have reviewed the patch and it looks fine to me. So if the philosophical question of "whether" is settled and the answer is "we should replace preprocess with templates", then this looks good to me.

I guess we should review the HTML markup before and after this patch and make sure it is exactly the same though? Just in case... since I'm not sure about the philosophical discussion from the comments above, I haven't done this...

lauriii’s picture

If we want to do things how we've planned to do I think we should create the template for search block. It just makes me a bit worried how are going to maintain all the changes for so many different templates. Maybe it becomes easier when we move the functionality to Classy and we don't have that much functionality in the module tempalates.

Not sure if its possible or good idea that we would use in cases like this Twig block and then change what we need to change by extending the template.

davidhernandez’s picture

This template, in particular, is fairly simple. Because I just copied the default block template, there isn't much markup besides a wrapper div where the attribute is printed, and an H2. I think the maintenance will be fairly low.

I think we should use extends (and blocks) more, but it probably wouldn't help here. The main difference would be with the wrapper div and that would require overriding pretty much the whole template.

star-szr’s picture

Status: Needs review » Needs work

I'm fine with this, just didn't want this to delay the original block issue (#2328913: Move block classes out of preprocess and into templates).

Let's use |without('role') though for now please, core needs to set a good example there. We can refactor once we have setAttribute :)

lauriii’s picture

Status: Needs work » Needs review
FileSize
550 bytes
3.06 KB
davidhernandez’s picture

The change in #9 is fine. Can someone else review and double-check the markup changes? It looked fine to me, but I made the original patch, so I don't want to RTBC it.

jhodgdon’s picture

Status: Needs review » Needs work

I tested a recent install with and without this patch.

The markup of the Search block is not exactly the same... There are two changes:
1) The order of the classes on the outer div is different. This is not important.
2) Before the patch, there is this div wrapping the form, and this is missing with the patch applied:

<div class="container-inline content">
...
</div>

I do not know why that div is there or why it isn't there with the patch, but it's a difference.

That aside... I really don't think this is a great idea at all:

a) It is much easier to maintain a four-line preprocess function than a 54 line twig template.
b) It is much harder to maintain multiple copies of block--*.html.twig than one copy of block.html.twig.
c) It is much harder for themers to override multiple copies of block--*.html.twig than one copy of block.html.twig.

===> This is just a bad idea. I am really against committing it. As this is a child issue, I'll also comment on the parent...

jhodgdon’s picture

Actually it doesn't look like this is related to the parent issue... I still vote for keeping this as a preprocess rather than separate template.

Jeff Burnz’s picture

@jhodgen I think we have to move away from the crutch of preprocess, as you point out it's very easy to use to achieve a DRY approach, but that's not the nature of the new system.

I see it quite differently, to me search block template is just a bit of wrapper code that allows me to style the search from elements, in a block. The fact that it's "another block template" is, for all intents and purposes, irrelevant to me. What is more relevant to a themer/front end developer is to start building a Drupal theme and find there is no template for such a thing. To me that is kind of weird.

Honestly to me the real fix here is removing the container-inline class, but thats not part of the "move classes to templates" remit, but if we don't make this template then we get stuck with an ugly class in preprocess and no guarantee that the class will be removed from core, for maybe the next five years or so, or however long I have been having to deal with it already etc.

davidhernandez’s picture

I'd have to look some more but the missing div might be some funny business in Bartik, because it does some overriding of the block.

Not sure there is much to maintain. Once core is released, the templates don't really change. Also, D7 does have a search-block-form template. For me, c) is the biggest issue. I'm not a fan of magical unicorn templates, and having to override it separately from the main block template could easily get annoying.

As Jeff points out, and I feel really stupid for not bringing it up, the search preprocess function only adds the container-inline class. Do we need it? It is just shifting the search button inline, but you could argue that is a style decision. Bartik already takes care of it itself, by adding container-inline to the interior content div.

If we can remove the class, the role can stay in the preprocess function, and that negates the need for a template.

jhodgdon’s picture

Regarding the missing div, I was comparing just with and without the patch. Possibly Bartik is only overriding the base block template and not the search template, thereby underscoring my comment in #11 (c)? If that is the case and you want to make this patch, you'd better make a Bartik override of the search block template as well as the base one.

davidhernandez’s picture

Assigned: Unassigned » davidhernandez

I have another idea I want to test.

davidhernandez’s picture

Assigned: davidhernandez » Unassigned
Status: Needs work » Needs review
FileSize
5.63 KB

How about this? Since the whole point of the banana stuff is to eventually get the classes out of core modules, I cut out the middle man and moved the template to Classy. Since container-inline is the only class being added, there is no need for the template in the search module. The role can stay in the preprocess function. This removes the classes from preprocess, and gets a template to people who want one. Since the template is in Classy, we can group it with the other block templates, giving it greater visibility, and reducing the unicorniness.

Stark loses the container-inline, which would happen in the long run, and Seven maintains it. I added a template for Bartik, since Bartik adds an interior div to all blocks. We could also do this in Bartik by just using an extends in the template file {% extends "@bartik/block.html.twig" %} and nothing else. That way all code changes to Bartik's block template are picked up automatically. The outer container-inline, though, would have to be added in Bartik's preprocess.

jhodgdon’s picture

Um... I don't get this. It looks like the search module's block template is exactly the same as the main one in the block module. I think Bartik can override the search template using template suggestions without there being a specific template in search module, right?

davidhernandez’s picture

The patch in #17 does not include a template in the Search module. It includes a template in the Classy theme, and a modified one in Bartik. The only change to the Search module is the removal of the class from preprocess.

jhodgdon’s picture

Ah, my mistake, sorry! OK, that looks reasonable. The templates in Bartik and Classy seem to differ only in how they set up classes at the top, which seems reasonable.

davidhernandez’s picture

+++ b/core/themes/bartik/templates/block--search-form-block.html.twig
@@ -0,0 +1,58 @@
+  {% block content %}
+    <div{{ content_attributes.addClass('content', 'container-inline') }}>
+      {{ content }}
+    </div>
+  {% endblock %}

The real difference between them is this. The core block template does not have this div inside of 'content'.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

OK, this looks good. I manually tested it in both Bartik and Classy themes, and the search form block looks fine in both, with the button inline with the form.

Updating issue summary to reflect what this issue is actually doing now.

Jeff Burnz’s picture

It's a good summary. So later on we could just nuke the template from the Classy theme, aka simply remove this container inline class by getting rid of the template, Bartik does what it does etc?

davidhernandez’s picture

There would be no follow up to this issue. The template in Classy exists specifically for those classes to be there, so it would stay long term. It also removes what is being done in the search module's preprocess, which is the ultimate goal.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Couldn't we use twig inheritance to make it easier to maintain the bartik block? http://twig.sensiolabs.org/doc/tags/extends.html

Something like

{% extends "core/themes/classy/templates/block--search-form-block.html.twig" %}
{% block content %}
  <div{{ content_attributes.addClass('content', 'container-inline') }}>
    {{ parent() }}
  </div>
{% endblock %}

Setting back to needs review to get opinion on whether this is possible. And maybe classy's search form could just extend the default block template?

davidhernandez’s picture

Well, I feel dumb. I don't know why I didn't pay attention to that twig block. For Bartik, yes, for Classy, I don't think so. What we are modifying doesn't have a defined block area in the original template. I'm gunna take a look and do some testing.

davidhernandez’s picture

Try this one. I had Bartik's extend Classy's. I couldn't find a way to extend the default block, since everything outside of the Twig block (content) is inaccessible. I'd like to revisit all of these when we're done, because I think all the templates could use some refactoring/strategizing, but there probably won't be enough time.

davidhernandez’s picture

I forgot to include the parent function.

joelpittet’s picture

Assigned: Unassigned » star-szr
Status: Needs review » Needs work
  1. +++ b/core/themes/bartik/templates/block--search-form-block.html.twig
    @@ -0,0 +1,23 @@
    +{% extends "@classy/block--search-form-block.html.twig" %}
    

    I think @Cottser has a patch going where this wouldn't have to explicitly declare which theme this is in. Not sure if it's ready, but assigning to him to have a look.

    If not it's not we can look at working on that together on Wed/Thurs at BADCamp and sort it out:)

  2. +++ b/core/themes/bartik/templates/block--search-form-block.html.twig
    @@ -0,0 +1,23 @@
    + * - content_attributes: An array of HTML attributes applied to the main content
    

    We shouldn't be using "An array" here, it should be A list for consistency with the coding standards:
    https://www.drupal.org/node/1823416#datatypes

  3. +++ b/core/themes/classy/templates/block--search-form-block.html.twig
    @@ -0,0 +1,54 @@
    + * - attributes: array of HTML attributes populated by modules, intended to
    

    Same here for the Array/list things and should start with a capital.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

Fixing the comments.

1. I think this is the issue. #2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders Not sure how close that is. We don't necessarily have to wait for it, since there are other instances of extend being used, and we can probably fix them all in one follow up issue.

star-szr’s picture

Assigned: star-szr » Unassigned

Agreed about the registry loader, that shouldn't hold this up.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
4.17 KB
1.58 KB

I just tested this in both the Classy and Bartik themes. The search block looks good in both, with the search button inline with the keywords field. I think all of the previous review comments have been addressed.

Looking at the documentation for the new Classy template, I saw a couple of very minor documentation standards problems (lists should be preceded by an explanation and a : ) so I fixed these in the attached patch, which I'm at least tentatively marking RTBC since the changes are very minor.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Agree that this rtbc - looks great. This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

davidhernandez’s picture

Assigned: Unassigned » davidhernandez
davidhernandez’s picture

Assigned: davidhernandez » Unassigned
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Added beta evaluation.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 84fad8a and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed 84fad8a on 8.0.x
    Issue #2358037 by davidhernandez, jhodgdon, lauriii: Add search form...

Status: Fixed » Closed (fixed)

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