Blocks are not rendered in order by weight

Ah, good catch, we just found this on #1927608-183: Remove the tight coupling between Block Plugins and Block Entities today

Regression from 7.x, so critical.

Added sorting of blocks in block_list function in core/modules/block/block.module.

I have tested this manually and can confirm the bug exists and this patch is working.

1. Moved 'Powered by Drupal' and 'Search' into the header
2. Block order: 'Powered by Drupal' -> 'Search'
3. Placed 'Powered by Drupal' after 'Search'
4. The order on the page does not change.
5. Applied patch from #3
6. Block order is updated: 'Search' -> 'Powered by Drupal'

This needs a web test to confirm the ordering.

@tim.plunkett - sorry about that.

In the attached patch, I have provided a test BlockRenderOrderTest for this issue. This patch also contains the code from #3.

Fixed the test description and a comment. Interdiff provided.

Tested the new patch in #7 manually using steps in #4 - broken without patch, works with patch.

I am guessing this also needs a code review though?

Needs issue summary update with steps to reproduce from #4 and remaining tasks with template from https://drupal.org/issue-summaries.

Updates issue summary

Summary is updated with steps to reproduce/test. Though I note that the original summary follows the 'bug report' template that shows when creating an issue.

Anything else required to get this RTBC?

I've reviewed this patch. It looks good, works as advertised, and I think it's ready to be committed.

Committed to 8.x. Thanks!

Follow up. Block Order tests failing randomly. Block weight setting not being applied correctly in WebTestBase.php

This fails 50% of the time in HEAD.
This passes 100% of the time locally.

This sort should really have been solved via the list controller's sort method. I'd love to see that be a static method, but haven't had time to fix it. We should probably revert the patch.

Eclipse

Reverted the original commit...

The list controller actually uses ConfigEntityBase::sort(), which is static already.

This fails 50% of the time in HEAD.
This passes 100% of the time locally.

I've seen this too, its passing and failing at random because the machine names of the blocks are dictating their rendered order, not the weight. We'll have to use a more advanced method in the test for checking for whether or not these things are truly rendering due to weight and not just coincidentally.

Working on a new test.

The issue that #14 fixes is that currently the weight is inside the settings array of the block entity, where it should be level with the settings array.

Okay, the new BlockRenderOrder test is happily failing after multiple local test runs. Sort is currently being set based on the block machine_name.

Just wanted to post this test here and see what the remote testbot thinks - we still need to actually fix the sorting in BlockListController::sort() - that work will come in the next patch.

Thanks everyone for their patience here...

Things are moving.

First, I've re-introduced the solution from #3... please continue reading:

Second, after talking to @tim.plunkett, I've moved the BlockListController::sort() to Block::sort()

The BlockRenderOrder test is passing locally, but only after modifying WebTestBase to include the weight settings during the drupalPlaceBlock method. (See #14)

I also don't understand the need to unset settings in drupalPlaceBlock - this was causing BlockRenderOrder to fail since we couldn't check for weight anymore. This may cause other tests to fail, but I cannot run the entire test suite locally so I'm willing to post this revision here and see what happens.

Regarding #16 we need to approach getting rid of block_list() going forward, but for now, we need it. Ideally, the static sort function is agnostic toward regions and doesn't do things like $regions = array_flip(array_keys($this->regions)); in order to do what we need. We should already have taken care of that logic before we start sorting blocks. IE: somewhere, somehow, loop through the regions and sort each region's blocks using sort- don't try to sort blocks by region and then by weight- its just messy and confusing to do it that way...

If the modifications to drupalPlaceBlock cause failures for other tests, we can possibly re-introduce the unset() and rewrite BlockRenderOrder to work around that problem, but I really can't think of a better way to determine that the blocks are rendering in the correct order by weight without being able to return that value from the settings array.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -373,10 +373,10 @@ protected function drupalPlaceBlock($plugin_id, array $settings = array()) {
+      'weight' => 0,
...
-      unset($settings[$key]);

Unfortunately, the weight key is actually stored outside of the settings, alongside region, status, etc.

This patch probably doesn't work in actual manual testing, but only passes because of this change to drupalPlaceBlock() which allows the test to pass.

It will need to work with this unset() call left intact.

Right, and its hard to do a manual test right now due to #2004650: Blocks don't save placement on the /admin/structure/block page, reverts to original state

I've tested manually just by changing the config files and its working. I second the 'Needs manual testing' tag - we need this tested by someone who can go in and confirm its working.

Attached is a new patch - after talking to @tim.plunkett I now understand why we need the unset() method in drupalPlaceBlock. I've added a comment where this is needed.

I've modified BlockRenderOrder to use weight provided directly by the $return_block.

Late interdiff based on the two patches, sorry. Not perfect but better than no interdiff.

Modified BlockRenderOrder to use ->get('weight') rather than ->weight.

#3: fixed_block_ordering-1987952-2.patch queued for re-testing.

+++ b/core/modules/block/block.module
@@ -460,6 +460,10 @@ function block_list($region) {
+    return $first->weight < $second->weight ? ($first->weight === $second->weight ? 0 : -1) : 1;

Sorry, if this is a dumb question, but I've looked at this a couple times now, and I cannot fathom how the $first->weight === $second->weight can ever be TRUE. It's only reached if $first->weight < $second->weight, right? I think the conditions should be switched. I also don't see any tests that test blocks with the same weight...

@tstoeckler you are totally correct about the uasort comparison. I've rewritten that.

From my tests, it seems that blocks with the same weight and label are parsed over alphabetically and then rendered on top of one another. ie: in reverse alphabetical order. Personally, I think that is fine since the typical workflow (using the admin/structure/block form) is via drag and drop so the weights should always be different for the 95% use case. It makes some sense that this would happen since the blocks are being traversed over and then rendered always at the beginning of the assigned weight position. We'd actually have to add even more to our sorting functions to make them render alphabetically by machine_name and I'm not sure that should hold up this issue.

If the two blocks have the same weight but different labels, I suppose one could argue that they should render alphabetically, but I still think this is a 10% use case problem and has more to do with our sort functions. As it stands, blocks with the same weight that are in the same region do render correctly and the order they should adhere to at that point is a bit subjective. Again, I'm thinking that is another issue entirely - in fact, the old title of the issue contained some opinion of "wrong" and really what we want to accomplish in this issue is ensuring that the blocks are rendered in order by weight.

With that, I present these modifications based on comment #31.

FYI: Just tested the patch with the D8 version of Block Up Down and it works, updating the module page accordingly.

Thanks !

Unfortunately there are still issues here. As an example I just started adding blocks to my sidebar left and ended up with very different sorts on the front end and back end:

https://www.evernote.com/shard/s4/sh/7bfab3d2-e871-4d58-a464-cce2fdf3545...

vs

https://www.evernote.com/shard/s4/sh/2c35e444-e933-45b5-bdf8-4f7a86d105d...

Afraid we need more work here yet. :-(

Eclipse

Links are not accessible :(

Attached the screenshots just in case.

Eclipse

Manual test workflow:

1. Installed latest Drupal 8 HEAD.
2. Moved all blocks to sidebar first (effectively re-creating https://drupal.org/files/Screen%20Shot%202013-06-05%20at%2011.03.07%20AM... from #36)
3. Pages show the blocks rendered out of order.
4. Applied patch from #32
5. Pages show the blocks rendered in order.

@EclipseGc - are you sure this error wasn't happening in your manual tests due to #1938898: Convert block theme tables to table #type?

See: https://drupal.org/node/1938898#comment-7461310
Also see #27

I think this is working fine - its just that the UI was still messed up from #1938898: Convert block theme tables to table #type not being resolved so your manual tests were failing.

It works, patch applies with offset

core8$ patch -p1 < fixed_block_ordering-1987952-32.patch
patching file core/modules/block/block.module
Hunk #1 succeeded at 455 (offset -5 lines).
patching file core/modules/block/lib/Drupal/block/BlockListController.php
patching file core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
Hunk #1 succeeded at 172 with fuzz 1 (offset 9 lines).
patching file core/modules/block/lib/Drupal/block/Tests/BlockRenderOrderTest.php
patching file core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
Hunk #1 succeeded at 363 (offset -5 lines).

And after patch

PS: sorry for noise

Lets get a reroll on this one...

git applyc https://drupal.org/files/fixed_block_ordering-1987952-32.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7193  100  7193    0     0   1198      0  0:00:06  0:00:06 --:--:--  5065
error: patch failed: core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php:163
error: core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php: patch does not apply

Cool. Here we go, Alex.

Tests passed.

back to rtbc

Needs another reroll...

git ac https://drupal.org/files/fixed_block_ordering-1987952-42.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7272  100  7272    0     0   4583      0  0:00:01  0:00:01 --:--:--  7087
error: patch failed: core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php:172
error: core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php: patch does not apply

still needs re-roll

Rerolled.

@oriol_e9g - thanks for the help but I have assigned this issue to myself to try and finish out. We have been working on it for a long time! Thanks.

Oh looks like someone finally went in and cleaned up ConfigStorageController's loading methods. This one passes locally.

Forgot diff...

RTBC again

Nice!

Committed 0e14311 and pushed to 8.x. Thanks!

Please open a new issue, this was fixed with test coverage in August.