Problem/Motivation

When the absolute value of #default_value greater than the absolute value of #delta in Weight form element can potentially cause overriding of the value on the next submit. This is the unexpected behavior of weight form element. The best example is Block Layout form where it causes unexpected weight overriding.

Steps to reproduce:

  1. go to Block Layout page;
  2. choose some region and block that will be placed;
  3. place chosen block into chosen region;
  4. repeate previous step 15 times;
  5. toggle show row weights;
  6. set weight to the minimum or maximum value for added blocks and save changes;
  7. remove one of the blocks from chosen region;

Expected: weight values of added blocks should not be changed.

Test in #13 shows that there is no issue with block API.

Proposed resolution

Add an option value for weight select element that would be equal to #default_value in process method.

Remaining tasks

Write the test for weight form element.

User interface changes

Weight select will contain additional option with value (egual to #default_value) that not included in rande of values based on #delta value generated by process method.

API changes

No changes.

Data model changes

No changes.

Original report by gpk

Deleting blocks can cause block order (weights) to be munged

The blocks admin form is generated by http://api.drupal.org/api/function/block_admin_display_form/6. If a block is deleted then the calculated value of $weight_delta (the "delta" value for the weights dropdown) will as often as not be reduced. However the actual block weights are not altered. This can result in blocks having invalid weights (#default_value) in the form definition.

Example: Suppose we have 40 blocks and delete one, so that $weight_delta is now 19. Since the first block in each region will (if tabledrag has been used) have weight -20, no value is output for the "weight" select element in the form. In most browsers this seems to result in the first option in the select being selected (i.e. -19). Hence 2 blocks in each region will now have the same weight.

The end result is that the blocks at the top of each region will start mysteriously re-arranging themselves.

This can be compounded by #565184: Blocks can be displayed in different order from that shown on blocks admin page.

CommentFileSizeAuthor
#38 interdiff-565220-35-38.txt574 bytesYogesh Pawar
#38 565220-38.patch2.15 KBYogesh Pawar
#35 interdiff-565220-27-35.txt1.35 KBvoleger
#35 form_system-fix_weight_form_element-565220-35-8.6.x.patch2.18 KBvoleger
#29 form_system-fix_weight_form_element-565220-29-7.x.patch595 bytesvoleger
#27 form_system-fix_weight_form_element-565220-27-8.6.x.patch2.13 KBvoleger
#27 interdiff-565220-19-27.txt518 bytesvoleger
#19 interdiff-565220-18-19.txt908 bytesvoleger
#19 form_system-fix_weight_form_element-565220-19-8.6.x.patch2.16 KBvoleger
#19 form_system-fix_weight_form_element-565220-19-8.6.x-test-only.patch1.43 KBvoleger
#18 interdiff-565220-17-18.txt673 bytesvoleger
#18 form_system-fix_weight_form_element-565220-18-8.6.x.patch1.93 KBvoleger
#18 form_system-fix_weight_form_element-565220-18-8.6.x-test-only.patch1.2 KBvoleger
#17 form_system-fix_weight_form_element-565220-17-8.6.x.patch1.92 KBvoleger
#17 form_system-fix_weight_form_element-565220-17-8.6.x-test-only.patch1.19 KBvoleger
#14 block-weight_issue-565220-14-8.6.x.patch821 bytesvoleger
#13 block-weight_issue-565220-13-8.6.x-test-only.patch2.39 KBvoleger
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

gpk’s picture

Component: blog.module » block.module

For info: $weight_delta was introduced in #293370: Block sort / reorder fails if more than 20 items.

Also correcting Component.

MtRoxx’s picture

I seem to be having this issue as well. When I try to move blocks, they jump around to other menus.

brad.bulger’s picture

I am not sure if this should go here, on to the related ticket, or a new issue, but I am seeing a pretty basic problem: the "weight" column in the blocks table is a tinyint. and on mysql at least, the maximum range of a tinyint is -128 to 128. but if you have more than 256 blocks, as we do, it tries to set a weight to values way outside that range, because $weight_delta is based on the total count of blocks. when they're saved to the db, it silently knocks them all back to the lower or upper bound. moving blocks around with drag and drop can result in all the blocks being set to a weight of -128. i don't see any further finer level values being stored anywhere, so it sure looks like the block order would be, what, alphabetical at that point? or by module and then delta?

gpk’s picture

@3: sounds like a different problem. Suggest you open a new issue if there isn't already one for your problem.

Neil C Smith’s picture

Version: 6.13 » 7.31
Priority: Normal » Critical
Issue summary: View changes

This issue is still in 7.31, just as described in the original summary. I recently disabled a number of now unused views on a long running site, which removed about 20 different blocks. This reduced the available deltas causing almost all the blocks to be assigned the same minimum delta on the block admin page, and thus all block orders to be completely randomized across the site when saved.

Marked up as critical since this is in effect data loss - there is no way to restore the old block ordering without reverting to a database backup or manually reconfiguring.

cilefen’s picture

Please check if this is a bug in Drupal 8. If so, the issue must be moved to the Drupal 8 queue and fixed there first according to the backport policy. If it is relevant to Drupal 8, move it to branch 8.0.x-dev and tag it “Needs backport to D7”.
There may also be an existing Drupal 8 issue. Try to find it. If one exists, tag it “Needs backport to D7” and mark this issue “Closed: duplicate”. If the Drupal 8 issue is open, you may offer a patch. If its status is “Fixed”, make its status “Patch (to be ported)”, move it to version 7.x-dev and upload your latest patch if you have one.

david.soendergaard’s picture

I can confirm that this is still a problem in Drupal 8.2.3.

When reordering the blocks, it will start the weight on -$delta_weight+1 for all the weights.
This weight will be constant when deleting blocks, except that when deleting more 3+ blocks, the top blocks will all start getting the same weight, until we hit the new $delta_weight

GiorgosK’s picture

Version: 7.31 » 8.2.6

its the seconds report (together with the 1 above) that this happened to

there was no problem before but after deleting 2 blocks and then tried to put them back the orders withing regions get screwed up
and end up in different positions than I intend to put them

if you make the weight visible after saving all blocks have the same weight "-32" and "-32" is the smallest number I can choose
NOTE there are 64 blocks active on the drupal installation

blocks can be placed on their right places using weights (not with drag and drop) but as soon as I drag/drop is enabled again and I save the blocks they change positions

I will report back with more findings

GiorgosK’s picture

Status: Active » Postponed (maintainer needs more info)

upon further investigation at least in our situation it was caused by a badly behaving admin theme

if more people have other reports please change the status

sassafrass’s picture

Status: Postponed (maintainer needs more info) » Active

Blocks mysteriously reorder for me. I am using D8.3.2 It seems to happen more often when using the drag and drop, rather than when using the weights...but it does happen at other times that users have reported editing blocks that are in the block layout page. More often than not, the Title block and the Tab block end up at the bottom of most pages and the rest of the blocks maintain their order, but not their weights. All weights are reassigned to be sequential. No blocks can seem to have the same weight.

sassafrass’s picture

Also... I have only experienced this in the Content Region.

catch’s picture

@sassafrass the original bug report here was only about weights being changed on block deletion. If you're getting unexpected behaviour when editing rather than deleting, would you mind opening a new issue and linking to it from this one?

voleger’s picture

Version: 8.2.6 » 8.6.x-dev
FileSize
2.39 KB

Steps to reproduce:

  1. Go to Block Layout page
  2. Choose some region and block that will be placed
  3. Place chosen block into chosen region
  4. Repeate previous step 15 times
  5. Toggle show row weights
  6. Set weight to the minimum or maximum value for added blocks and save changes
  7. Remove one of the blocks from chosen region

Expected: weight values of added blocks should not be changed.

Attached test shows that there is no issue with block API. So we should test Block Layout form in the Javascript based test.

voleger’s picture

Status: Active » Needs review
FileSize
821 bytes

I figured out that it is the problem with processing weight element.
And especially this related to the situation when weight element process select element with calculated options based on #delta value.
So, when the user in block layout create a lot of blocks and define maximum or minimum weight value, then that value records into block entity.
After that user deletes some block in the region. In that situation weight (select) field rendered with the default value from block entity field and that value is not presented in the range of options of weight (select) field. So closest value to the default value will be chosen for that field. And that value will replace previous weight value if the user submits Block layout form again.
That patch checks that situation and adds to the options list value from #default_value property if it is required.

tim.plunkett’s picture

Component: block.module » forms system
Priority: Critical » Major
Issue tags: +Needs tests, +Needs issue summary update

Interesting find!
This needs a new title now that it's not directly tied to the Block system.

voleger’s picture

Title: Deleting blocks can cause block order (weights) to be munged » Fix Weight form element behavior
Issue summary: View changes
Issue tags: -Needs issue summary update

voleger’s picture

So, we have a patch and the test is green. Needs review.

Manuel Garcia’s picture

Test looks good to me, only one tiny nitpick

+++ b/core/tests/Drupal/Tests/Core/Render/Element/WeightTest.php
@@ -0,0 +1,54 @@
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = ['system'];

We could probably just use {@inheritdoc} here.

voleger’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! and covered with tests (test-only #19)

voleger’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: form_system-fix_weight_form_element-565220-29-7.x.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

Status: Needs work » Reviewed & tested by the community

@voleger please file separate issue for d7 backport

andypost’s picture

Issue tags: +needs backport to D7
voleger’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Render/Element/Weight.php
@@ -59,6 +59,10 @@ public static function processWeight(&$element, FormStateInterface $form_state,
+      if (!in_array($element['#default_value'], $weights)) {
+        $weights[$element['#default_value']] = $element['#default_value'];

in_array() has some pretty strange behaviour with different data types. I think we should be a bit more defensive about what #default_value is. Also since the value is also the key we can use isset() which will be faster. I suggest that we cast #default_value to an integer before doing anything. So the code should look something like:

$default_value = (int) $element['#default_value'];
if (!isset($weights[$default_value])) {
  $weights[$default_value] = $default_value;
  ksort($weights);
}
voleger’s picture

Good point.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Render/Element/Weight.php
@@ -59,10 +59,12 @@
+      unset($default_value);

I think this is unnecessary.

Yogesh Pawar’s picture

Assigned: Unassigned » Yogesh Pawar
Yogesh Pawar’s picture

Assigned: Yogesh Pawar » Unassigned
Status: Needs work » Needs review
FileSize
2.15 KB
574 bytes

Changes done as mentioned in #36 & also added an interdiff.

voleger’s picture

+1 for RTBC

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Yes last unset is useless)