Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

D7 crossport: Reordering fails with more than 100 items in a menu #5400

Closed
klonos opened this issue Dec 15, 2021 · 6 comments · Fixed by backdrop/backdrop#3866
Closed

D7 crossport: Reordering fails with more than 100 items in a menu #5400

klonos opened this issue Dec 15, 2021 · 6 comments · Fixed by backdrop/backdrop#3866

Comments

@klonos
Copy link
Member

klonos commented Dec 15, 2021

This is the respective issue as https://www.drupal.org/project/drupal/issues/1007746, with https://git.drupalcode.org/project/drupal/-/commit/bf9fbe42f38ff9ab043703e8cc22984861893077 being the change that was merged into D7 back in June (which was a backport of what went into D8):

Problem/Motivation

An issue occurs when more than 100 items exist in the root of the menu or under a single parent item, or items exist with a weight outside of -50..50 (causing overflow when trying to fit that weight into the select form element).

Proposed resolution

The main problem is that the delta value of the weight "property" is hardcoded to 50, so if we have more than 100 elements (-50 to 50) we won't be able to sort items in the menu for higher weights.

The solution is to query the database the min and max values from the weight field in the menu_links table, then make the abs value (see detailed explanation at #96).

The last patch pass all tests on all relevant PHP and MySQL releases.

@klonos klonos self-assigned this Dec 15, 2021
@klonos
Copy link
Member Author

klonos commented Dec 15, 2021

https://www.drupal.org/project/drupal/issues/1007746#comment-5596710

Show row weights is at the heart of the issue. The html select element (dropdown menu) for the row weight is created based on the "delta" value discussed in the thread above. That value is currently hardcoded at 50, so the select element ranges from -50..50. When you have an element with a weight larger than 50, it's an overflow error, and making any modifications to the menu from the editor interface will overwrite the out-of-bounds row weights.

I've updated my patch to include backportability and added test cases (thanks xjm). The new test cases (I just added them to menu.test) try to create a menu of 102 items and assign increasing weights to each element, and fails when trying to set a weight of 51 because the select element caps at 50 (because delta=50). The patched code (in menu.admin.inc and menu.module) removes the hardcoded 50, and adjusts it to match the total number of items already in a menu (or 50 if there are less than 50 items).

@herbdool
Copy link

RTBC. Looks good! I tested and it works as expected. And the code looks good.

@klonos
Copy link
Member Author

klonos commented Jan 16, 2022

Thanks @herbdool 🙏🏼

...I've rebased the PR branch just in case - no code changes, but we now have failing tests. I'll work on that soon as I get a chance.

@jenlampton jenlampton modified the milestones: 1.21.1, 1.21.2 Jan 21, 2022
@herbdool
Copy link

Maybe rebase again in case the work on menu in 1.21.1 fixes things. @klonos

@klonos klonos modified the milestones: 1.21.2, 1.21.3 Feb 16, 2022
@jenlampton jenlampton modified the milestones: 1.21.3, 1.21.4 Mar 2, 2022
@jenlampton jenlampton modified the milestones: 1.21.4, 1.21.5 Mar 16, 2022
@herbdool herbdool modified the milestones: 1.21.5, 1.22.1 May 16, 2022
@jenlampton jenlampton modified the milestones: 1.22.2, 1.22.3 Jul 20, 2022
@jenlampton jenlampton modified the milestones: 1.22.3, 1.23.1 Sep 15, 2022
@jenlampton jenlampton modified the milestones: 1.23.1, 1.23.2 Dec 2, 2022
@quicksketch quicksketch modified the milestones: 1.23.2, 1.24.1 Jan 15, 2023
@jenlampton jenlampton modified the milestones: 1.24.1, 1.24.2 Mar 15, 2023
@jenlampton jenlampton modified the milestones: 1.24.2, 1.24.3 Apr 19, 2023
@klonos klonos modified the milestones: 1.24.3, 1.25.1 Jun 6, 2023
@quicksketch quicksketch modified the milestones: 1.25.1, 1.25.2 Jun 7, 2023
@klonos
Copy link
Member Author

klonos commented Jun 16, 2023

Sorry, this has slipped under the radar. I have rebased (no other changes), and the PHP failures are gone, and no PHPCS/cspell complains either.

Setting this back to RTBC, based on @herbdool's previous review.

backdrop-ci referenced this issue in backdrop/backdrop Sep 13, 2023
By @klonos and @herbdool.

Port of Drupal Issue #1007746 by figureone, dawehner, tunic, TuWebO, voleger, ryan.gibson, mcdruid, tim.plunkett, hefox, Ranko, joseph.olstad, xjm, Fabianx, David_Rothstein, chrisgross, gmclelland, webchick.
backdrop-ci referenced this issue in backdrop/backdrop Sep 13, 2023
By @klonos and @herbdool.

Port of Drupal Issue #1007746 by figureone, dawehner, tunic, TuWebO, voleger, ryan.gibson, mcdruid, tim.plunkett, hefox, Ranko, joseph.olstad, xjm, Fabianx, David_Rothstein, chrisgross, gmclelland, webchick.
@quicksketch
Copy link
Member

Thanks @klonos and @herbdool! Sorry this took so long to get reviewed. I merged backdrop/backdrop#3866 into 1.x and 1.25.x.

@klonos klonos removed their assignment Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants