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:
- go to Block Layout page;
- choose some region and block that will be placed;
- place chosen block into chosen region;
- repeate previous step 15 times;
- toggle show row weights;
- set weight to the minimum or maximum value for added blocks and save changes;
- 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.
Comment | File | Size | Author |
---|---|---|---|
#50 | 565220-50.patch | 674 bytes | alexpott |
#38 | interdiff-565220-35-38.txt | 574 bytes | yogeshmpawar |
#38 | 565220-38.patch | 2.15 KB | yogeshmpawar |
#19 | form_system-fix_weight_form_element-565220-19-8.6.x-test-only.patch | 1.43 KB | voleger |
Comments
Comment #1
gpk CreditAttribution: gpk commentedFor info:
$weight_delta
was introduced in #293370: Block sort / reorder fails if more than 20 items.Also correcting Component.
Comment #2
MtRoxx CreditAttribution: MtRoxx commentedI seem to be having this issue as well. When I try to move blocks, they jump around to other menus.
Comment #3
brad.bulger CreditAttribution: brad.bulger commentedI 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?
Comment #4
gpk CreditAttribution: gpk commented@3: sounds like a different problem. Suggest you open a new issue if there isn't already one for your problem.
Comment #5
Neil C Smith CreditAttribution: Neil C Smith commentedThis 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.
Comment #6
cilefen CreditAttribution: cilefen commentedPlease 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.
Comment #7
david.soendergaard CreditAttribution: david.soendergaard commentedI 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
Comment #8
GiorgosKits 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
Comment #9
GiorgosKupon 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
Comment #10
sassafrass CreditAttribution: sassafrass as a volunteer commentedBlocks 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.
Comment #11
sassafrass CreditAttribution: sassafrass as a volunteer commentedAlso... I have only experienced this in the Content Region.
Comment #12
catch@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?
Comment #13
volegerSteps to reproduce:
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.
Comment #14
volegerI 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.
Comment #15
tim.plunkettInteresting find!
This needs a new title now that it's not directly tied to the Block system.
Comment #16
volegerComment #17
volegerHere's the test.
Comment #18
volegerSwitched to KernelTestBase
Comment #19
volegerOops, wrong namespace.
Comment #25
volegerSo, we have a patch and the test is green. Needs review.
Comment #26
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedTest looks good to me, only one tiny nitpick
We could probably just use
{@inheritdoc}
here.Comment #27
voleger#26 thanks, fixed.
Comment #28
andypostLooks good! and covered with tests (test-only #19)
Comment #29
volegerD7 patch
Comment #31
andypost@voleger please file separate issue for d7 backport
Comment #32
andypostComment #33
volegerHere is backport issue: #2946801: [D7] Fix Weight form element behavior
Comment #34
alexpottin_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:Comment #35
volegerGood point.
Comment #36
alexpottI think this is unnecessary.
Comment #37
yogeshmpawarComment #38
yogeshmpawarChanges done as mentioned in #36 & also added an interdiff.
Comment #39
voleger+1 for RTBC
Comment #40
andypostYes last unset is useless)
Comment #41
alexpottAdding credit for reviews that influenced the patch.
Comment #42
alexpottCommitted cf94492 and pushed to 8.6.x. Thanks!
Set to ptpb for cherry-pick to 8.5.x once 8.5.0 is out.
Comment #44
alexpottCommitted 8d6c986 and pushed to 8.5.x. Thanks!
Someone can open the 7.x issue if they like.
Comment #46
volegerBackport issue
#2946801: [D7] Fix Weight form element behavior
Comment #47
neclimdulPatch added
Comment #48
neclimdulPatch added
Comment #49
neclimdulPatch added kernel test(weight test) to unit test suite cause it to fail.
Comment #50
alexpottHere's a patch. Tested locally all works.
Comment #51
alexpottCommitted and pushed b175df6f61 to 8.6.x and 3a2f18daea to 8.5.x. Thanks!
Comment #54
neclimdulThanks so much!