Convert block theme tables to table #type

Hi,

I had a quick look at this, and I'm not sure that the table #type is quite up to the challenge of converting this table yet? (And possibly it's beyond the scope of the #type to do so?)

I couldn't see a way of passing the colspans required to generate the headers for each region.

It looks like you would have to split this up into smaller tables (one for each region) and lose the sticky header that runs over the whole list. Which is probably not desirable...

Thanks,
Pete

Hi @Pete B thanks for having a crack. Though the solution isn't self evident I think it's to use
#wrapper_attributes

#1948374: #type 'table' allow attributes on table cells

Personally, I would prefer if it just treated #rows like it's #theme =>table wrapper does but there are reasons why it doesn't so I digress.

Do you think that would do the trick?

Joelpittet,

Yes that does do the trick! If $form['blocks'] is a table #type, You can do something like:

    $form['blocks']['myregion']['header'] = array(
      '#markup' => 'My markup',
      '#wrapper_attributes' => array('colspan' => 4),
    );

To create a colspan for the header. Awesome!

Would be great to get an example on the documentation page. http://drupal.org/node/1876710

Thanks for the pointer!

Thanks,
Pete

#8: 1938898-8-block-table.patch queued for re-testing.

Looking really nice... You actually can use no_striping as an #attribute. I have added this in.

The patch now matches the style of the original version. I also fixed some small coding standards on the tabledrag lines.

Thanks,
Pete

#10: block-admin-to-type-table-1938898-10.patch queued for re-testing.

Re-rolled to apply despite the twig changes underneath, which fixes bugs introduced in the twig version too. Now to profile!

Performance profile is good! In summary:

Number of Function Calls 47,650 42,554 -5,096 -10.7%
Incl. Wall Time (microsec) 265,732 238,642 -27,090 -10.2%
Incl. CPU (microsecs) 200,012 176,011 -24,001 -12.0%
Incl. MemUse (bytes) 10,377,136 10,423,928 46,792 0.5%
Incl. PeakMemUse (bytes) 10,592,192 10,603,184 10,992 0.1%

Full XHProf profile attached. Using theme_table may increase memory usage slightly but is overall significantly faster.

The twig-ified block admin form before this patch appears broken to people, but it is in fact functional despite the messed up display; therefore tests relating to it were still passing. Further test coverage is *not* needed; we already know our tests can't catch all visual problems.

I think this looks great. RTBC, but wait for the bot to come back.

Committed d1a88e9 and pushed to 8.x. Thanks!

Manual tests are showing changes to weight are not applying. I can move blocks around from region to region and save that, but the changes I make ordering by weight do not save (ie: change a weight from -2 to -4, click save, the block weight remains as -2). So we are still blocked for manual tests on #1987952: Blocks are not rendered in order by weight

Edit: I dug a bit further and I'm actually seeing that ALL blocks show up on admin/structure/block as having a weight of -2 now. However, when I look at the config files, the weight changes are being saved.

Edit2: Stood over @tim.plunkett's shoulder and watched him re-create the error but he is seeing all blocks as -4 and cannot see any changes persist when saving weight changes. Inspecting the config file showed that the weight changes were not being saved in the yml either.

I RTBC'd this, I apologize, here's the fix.
The foreach loop was busted. It was using the $entity outside of the foreach, which explains why all blocks showed the same weight.

So... one thing that has occurred to me as I begin writing tests for the block admin ui is that we won't be able to assert blocks are showing the correct weight in the ui without the modifications we're making to drupalPlaceBlock in WebTestBase with #1987952: Blocks are not rendered in order by weight. So it would seem there is some circular dependencies going on here. I suppose the thing to do would be to make the changes to drupalPlaceBlock in a patch here and then reroll #1987952: Blocks are not rendered in order by weight without those changes?

Edit: Nevermind... patch including test coming.

It would appear that BlockUiTest was empty. The attached should provide some more coverage - including what's relevant for @tim.plunkett's patch in #21.

Sorry about the errors in #23 - this one should be better.

Looks good to me. The test extensions are especially welcome!

Conflicted with #1980338: Fatal error: Class 'plugin.manager.block' not found in /core/modules/system/system.module on line 1162, here's a reroll, no changes.

Committed 1936dbc and pushed to 8.x. Thanks!