Move block classes out of preprocess and into templates

I'll look into this now.

Ok so, here's a patch that removes classes from template_preprocess_block and adds them to block twig template. I also fixed tests that failed.

There's still stuff to do with blocks that wants to add more classes. Pretty much system_preprocess_block and bartik_preprocess_block are only ones. I couldn't really figure out how system_menu_block works so kinda need help with that.

Yes, I was thinking about that but there's a separate issue for it http://drupal.org/node/1972122

I don't think this should be blocked on #1972122: Remove the DIV tag around block content, this is much more straightforward and either one can be rerolled :)

I think we should focus on template_preprocess_block() and block.html.twig in this issue.

system_preprocess_block() and bartik_preprocess_block() are such special cases, if we want to add more templates for those that's fine but not sure it has to happen in this issue.

search_preprocess_block() should be left as is IMO, it's an optional module. If we want to pull that out it and create a template it can be its own issue.

Thanks for working on this @lanchez!

Just moved content class to twig. This might fail tests since class order did change on some blocks.

So should we do more in this issue or not?

I think we can fix up the tests and wrap this one up. Thanks @lanchez!

Now the test is "fixed" if you know what I mean. The classes should be checked from rendered block, but I'm not very good with tests. So either it's fine like that or someone should the test better.

@lanchez Nice work! That fail seems to be on the content class test. Do you think you can try debugging that?

The issue summary suggests we should look at all those preprocesses and templates suggestions. There is a chance we don't need to do anything for those but we should address them never the less in-case we miss something.

Wow sorry, I cross posted with you and all sorts of things went weird. Here's a re-upload for testbot.

Looks good! I manually tested and things (attribute order and class order) moved around a bit but nothing of consequence.

+++ b/core/modules/block/src/Tests/BlockPreprocessUnitTest.php
@@ -50,7 +50,7 @@ function testBlockClasses() {
-    $this->assertEqual($variables['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes');
+    $this->assertEqual($variables['content_attributes']['class'], array('test-class'), 'Test-class class added to block content_attributes');

Just an FYI regarding the test. It's correct the way you did that. I didn't think so at first but it's testing what comes of the preprocess and that is not in the preprocess, so to test that you'd have to render the block and do xpath or something. Though the plan is to remove that class anyways so... that'd be total busy work... RTBC +1

Oh wait, @Cottser thoughts on #13 second paragraph regarding the issue summary?

Actually yeah it needs work,

function search_preprocess_block(&$variables) {
  if ($variables['plugin_id'] == 'search_form_block') {
    $variables['attributes']['role'] = 'search';
    $variables['content_attributes']['class'][] = 'container-inline';
  }
}

Needs to be moved into a search template, and the block classes need to be added there as well.

As well as the other items mentioned in the issue summary.

@joelpittet I addressed that in #7. I think if we want to go that far and add all those templates (don't forget we need to add them to hook_theme() as well if they are new module templates) then IMO that should happen in a separate issue.

Personally I think things like search_preprocess_block() are not defining default classes but rather are acting the way contrib will, just adding classes to the default list. I think we should be focussing more on template_preprocess_* functions, that is where we will have the biggest impact. Because the reality is there are classes coming from other places than preprocess so this initiative as it is will not be removing all classes from core markup.

@Cottser I get ya on the preprocess, what about system branding block, we've lost classes on those blocks that were previously there. Which I'd expect a visual regression,no?

Will check for myself after breakfast:p

Ok Updated the issue summary, and back to RTBC. The other blocks are using @extend :)

Rerolled

Looks good. Patch still applies. Back to RTBC.

Let's open a followup to get rid of BlockPreprocessUnitTest it looks completely pointless.

Committed 4d2c54d and pushed to 8.0.x. Thanks!

Followup issue created #2339069: Remove useless BlockPreprocessUnitTest