Using nested paragraphs we ran into the the issue that the weights were calculated wrong, after drag and dropping a paragraph that contained other paragraphs. What was wrong about the weights was, that they had the same values.
This occurred because of
first, how the updateField behavior in tabledrag.js is handling ordering, assigning the max value when running out of values
case 'order':
{
var siblings = this.rowObject.findSiblings(rowSettings);
if ($(targetElement).is('select')) {
var values = [];
$(targetElement).find('option').each(function () {
values.push(this.value);
});
// only takes into account the values count on one level
var maxVal = values[values.length - 1];
// targetClass is field_name-delta-order and this function takes into account all values on every level
$(siblings).find(targetClass).each(function () {
if (values.length > 0) {
this.value = values.shift();
} else {
this.value = maxVal;
}
});
and second, of how the order class is set in theme.inc inside the template_preprocess_field_multiple_value_form function
function template_preprocess_field_multiple_value_form(&$variables) {
$element = $variables['element'];
$variables['multiple'] = $element['#cardinality_multiple'];
$variables['attributes'] = $element['#attributes'];
if ($variables['multiple']) {
$table_id = Html::getUniqueId($element['#field_name'] . '_values');
// setting it up like doesn't take into account that multiple values on the same content could have the same field name
$order_class = $element['#field_name'] . '-delta-order';
this leaves room for same name order errors, because we choose a popular name like 'field_body' for a paragraph field that contained other paragraphs.
Assigning a unique name to the order class using the unique table id instead of the field_name only, like
if ($variables['multiple']) {
$table_id = Html::getUniqueId($element['#field_name'] . '_values');
$order_class = table_id . '-delta-order';
The reason behind this approach is that the table id already contains the field_name and also adds a unique part if the content is already on the view.
Is this a good approach or have I missed something similar in the issue queue?
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | 3098960-saved.png | 148.78 KB | godotislate |
| #24 | 3098960-edit.png | 217.58 KB | godotislate |
| #24 | 11.x-saved.png | 149.12 KB | godotislate |
| #24 | 11.x-edit.png | 217.61 KB | godotislate |
| #2 | nested-content-with-same-field-name-sorting-3098960-1.patch | 715 bytes | schaefdi |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !9897
Comments
Comment #2
schaefdi commentedComment #3
schaefdi commentedComment #4
candalt commentedCan confirm this behaviour. Have tested on a fresh installation:
Drupal: 8.8.5
Profile: Standard
Paragraphs: 1.11
Add paragraph "Child" with field "Title"
Add paragraph "Parent" with field "Title" and "Modules (field_modules)", relation to "Child" (many)
Add field "Modules (field_modules)" to "Article" nodetype, relation to paragraph "Parent"
Try to add multiple paragraphs of type "Parent" to "Article". After adding, drag and drop one paragraph to change order.
Result: Weight for all paragraphs are set to "0,1,1" and so on.
Comment #5
abalevik commentedCan confirm that patch #2 fixes the problem mentioned in the issue, which @candalt mentions.
Great if anyone else can confirm / test as well.
Comment #6
candalt commentedHave tested patch #2 and it is working fine.
Comment #8
kevinquillen commentedThe patch applies, but I see the same error. The console has several "Error: Cannot find valid paragraph weight to adjust!" within a Layout Builder edit interface for a block with nested paragraphs.
Comment #9
avpadernoDrupal 8.9.x is now open only to security issues.
Comment #11
fabsgugu commentedPatch tested, Work on version 9.3.3 for me.
Comment #14
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
This seems more like a bug potentially
Either way will need a test case
Did not test.
Comment #15
raphael apard commentedPatch tested, work on version 9.3.2 for me.
Comment #16
mediabounds commentedPatch in #2 resolved the issue for me!
Comment #18
pbonnefoi commentedPatch worked for me on Drupal 10.3 and Drupal 11.
Comment #20
pbonnefoi commentedComment #21
smustgrave commentedPreviously tagged for tests which still appear to be needed. thanks
Comment #23
arunkumarkAs per the comment, I added a test case to check the unique class for the multi-field value. Moving to NR now.
Comment #24
godotislateI did some manually testing. On latest 11.x, after following the reproduction steps in #4 to create the paragraphs and fields, I created Article content with 3 Parent Paragraphs (Parent 1, Parent 2, and Parent 3), and each Parent Paragraph with 2 children (Parent 1 => Child 1, Child 2; Parent 2 => Child 3, Child4; Parent 3 => Child 5, Child 6). I then dragged Parent 3 to go above Parent 2 (See screenshot). After saving, Parent 2 incorrectly remained above Parent 3 (See screenshot).
I then applied the changes from MR 9897. Re-edited the page and dragged Parent 3 above Parent 2 again (see screenshot). After save, Parent 3 stays correctly above Parent 2 (See screenshot).
I tried to reproduce the issue mentioned in #8 with block content and nested paragraphs in Layout Builder, but I did not see the console errors.
The automated test that's needed is a functional test that does similar to steps to the manual test just described. But I'm not sure an automated test can be written using core alone, at least not easily. Given that and the fact that the fix is essentially a one-line diff, I wonder if it qualifies under #2972776: [policy, no patch] Better scoping for bug fix test coverage as something that can be committed without this test. Moving to RTBC for confirmation.
Comment #25
pbonnefoi commentedThanks for your help ! Indeed, it seems rather difficult to write a functional test since this bug is a combination of core and paragraphs.
Comment #27
nod_saving credits
Comment #28
nod_the test can be removed here. it's testing that something we removed was removed
Comment #29
godotislateUnnecessary test removed and rebased MR.
Comment #30
smustgrave commentedWith test coverage removed seems pretty straight forward fix then.
Comment #34
nod_Committed and pushed 6265e217854 to 11.x and 94d0636ceaf to 11.1.x. Thanks!