Problem/Motivation

There is a db_transaction(); in MenuRouterRebuildSubscriber

The problem with that is that nothing ensures that the menu router is actually stored in the database.

Note that this does not break anything and the issue is just cleanup.

Proposed resolution

  1. Change the definition of plugin.manager.menu.link in core.services.yml to pass in @database and store it in a connection. property of MenuTreeStorage.
  2. Move the code block in MenuRouterRebuildSubscriber::menuLinksRebuild() starting with $transaction = db_transaction(); into MenuTreeStorage::rebuild. Keep the lock mechanism in MenuRouterRebuildSubscriber.
  3. Change the db_transaction you just moved into $this->connection->startTransaction.
  4. Optional and can be a followup: write a unit test for MenuLinkManager::rebuild. AFAIK there is no unit test currently that could be easily extended to include this method and so this part is not a novice task (but, feel free to try. You'd need to mock the plugin discovery, the module handler, the link override and the tree storage, I would borrow ideas/code from LocalTaskManagerTest ).

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#62 2396619-62.patch1.83 KBYogesh Sahu
#54 interdiff-51-54.txt509 byteshardik_patel_12
#54 2396619-54.patch1.78 KBhardik_patel_12
#53 8.9.x-dev.png83.33 KBrajandro
#51 2396619-51.patch1.74 KBhardik_patel_12
#40 2396619-40.patch1.7 KBajits
#35 interdiff-32-35.txt514 bytesmitrpaka
#35 2396619-35.patch1.71 KBmitrpaka
#32 interdiff-27-32.txt411 bytesmitrpaka
#32 2396619-32.patch1.68 KBmitrpaka
#27 interdiff-21-27.txt2.17 KBmitrpaka
#27 2396619-27.patch1.67 KBmitrpaka
#21 2396619-21.patch2.39 KBacrosman
#20 2396619-20.patch.txt2.39 KBacrosman
#16 2396619-16.patch2.51 KBrpayanm
#12 remove_database_dependency_as_commented_-2396619-12.patch2.43 KBvj

Issue fork drupal-2396619

Command icon Show commands

Start 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:

About issue forks

Comments

chx’s picture

chx commented
Issue summary: View changes
chx’s picture

chx commented
Issue summary: View changes
chx’s picture

chx commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented

If you would call the rebuild for your own, you should not have to take care about setting up the lock, starting a transaction etc.
The MenuRebuildSubscriber code should also be entirely independent from the way how the menu tree is stored, so
alternative number 2 is the WINNER!

chx’s picture

chx commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented
Assigned: pwolanin » Unassigned

We agreed on things, let's skip the assignment

chx’s picture

chx commented
Assigned: Unassigned » chx
dawehner’s picture

dawehner
Primary language German
commented

@chx
Are you still into it? Maybe we could mark the issue as novice, so someone else has some fun working on it.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes

Maybe this is a bit better, more verbose.

chx’s picture

chx commented
Assigned: chx » Unassigned
Issue summary: View changes
Issue tags: +Novice
chx’s picture

chx commented
Issue summary: View changes
vj’s picture

vj commented
Status: Active » Needs review
StatusFileSize
new remove_database_dependency_as_commented_-2396619-12.patch2.43 KB

I have created a patch by following the steps in "Proposed resolution".

I have not created tests, please guide me how to create it. Also let me know if any changes required in the patch.

Status: Needs review » Needs work

The last submitted patch, 12: remove_database_dependency_as_commented_-2396619-12.patch, failed testing.

chx’s picture

chx commented

Thanks for getting this one done! I am sorry for omitting this step -- but you still need to call $this->menuLinkManager->rebuild(); from MenuRouterRebuildSubscriber::menuLinksRebuild().

As for the test, as I said, let's leave that to a followup, that's a slightly bit too complicated for a Novice task, I think.

dawehner’s picture

dawehner
Primary language German
commented

I think its fair to not require an additional test for just the moving, given that we have some dort of general integration for menus all over the place already.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new 2396619-16.patch2.51 KB

Let me try.

Status: Needs review » Needs work

The last submitted patch, 16: 2396619-16.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/core.services.yml
@@ -397,7 +397,7 @@ services:
-    arguments: ['@menu.tree_storage', '@menu_link.static.overrides', '@module_handler']
+    arguments: ['@menu.tree_storage', '@menu_link.static.overrides', '@module_handler', '@database']

    ... I don't think the menu.link plugin manager needs the database. 

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php
@@ -59,19 +59,8 @@ public function onRouterRebuild(Event $event) {
     if ($this->lock->acquire(__FUNCTION__)) {
...
       $this->lock->release(__FUNCTION__);

    Having the lock here, in the middle of nowhere, is kinda pointless. Better wrap the ->rebuild() of the menu link manager.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented

I'm at DrupalCon LA, and working on moving this forward.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented
StatusFileSize
new 2396619-20.patch.txt2.39 KB

Rerolled that patch from #16 to try to take into account the suggestions given in response. I wasn't clear if the second suggestion was to move the call to rebuild() into the locking logic, or move the logic into the method. My instinct is that it should be in the method, and so I moved it there.

acrosman’s picture

acrosman
Primary language English
Location South Carolina, US
commented
StatusFileSize
new 2396619-21.patch2.39 KB

Replacing the file from 20 with a file with the proper file extension.

willzyx’s picture

willzyx commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: 2396619-21.patch, failed testing.

msgph’s picture

msgph commented

I'm in DrupalCon Barcelona and I will try to fix the failing patch

Contrib task: https://www.drupal.org/contributor-tasks/create-patch

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Issue tags: +Barcelona2015

I'm mentoring at DrupalCon Barcelona on that issue.
@msgph you might have a look at the contributor task page about writing an automated test too.

msgph’s picture

msgph commented
Issue summary: View changes

Just updating the summary for now, the first proposed solution has been done in the first patch #12. Second proposed solution needs some work.

mitrpaka’s picture

mitrpaka commented
Status: Needs work » Needs review
StatusFileSize
new 2396619-27.patch1.67 KB
new interdiff-21-27.txt2.17 KB

Patch attached based on the first proposed solution and original proposed resolution. Lock mechanism kept in MenuRouterRebuildSubscriber as proposed originally (as MenuTreeStructure doesn't have lock backend defined at the moment). Not sure whether should be added or not?

Patch created in co-operation with @msgph in DrupalCon Barcelona sprint.

Status: Needs review » Needs work

The last submitted patch, 27: 2396619-27.patch, failed testing.

The last submitted patch, 27: 2396619-27.patch, failed testing.

pwolanin’s picture

pwolanin commented

Do we really even need to call db_ignore_replica() ?

It seems that affects the $_SESSION, so is only for the current user in any case.

mitrpaka’s picture

mitrpaka commented
mitrpaka’s picture

mitrpaka commented
Status: Needs work » Needs review
StatusFileSize
new 2396619-32.patch1.68 KB
new interdiff-27-32.txt411 bytes

Updated patch to pass all tests trial ...

Status: Needs review » Needs work

The last submitted patch, 32: 2396619-32.patch, failed testing.

The last submitted patch, 32: 2396619-32.patch, failed testing.

mitrpaka’s picture

mitrpaka commented
Status: Needs work » Needs review
StatusFileSize
new 2396619-35.patch1.71 KB
new interdiff-32-35.txt514 bytes

Updated patch to pass all tests ...

Status: Needs review » Needs work

The last submitted patch, 35: 2396619-35.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ajits’s picture

ajits
he/him
Primary language Marathi
Location India
commented
Status: Needs work » Needs review
StatusFileSize
new 2396619-40.patch1.7 KB

Patch was outdated. Re-rolled against the latest head on 8.3.x
Did not look at failing tests yet.

Status: Needs review » Needs work

The last submitted patch, 40: 2396619-40.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs issue summary update

Regarding point 4 in the issue summary, we have some test failures, so understanding them might help to judge whether we need sort of unit test / more educated test.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vacho’s picture

vacho commented
Issue tags: +Needs reroll
vacho’s picture

vacho commented

When I review an tried to reroll, I noticed that this problem is already solved in code.

At file /core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php:75

/**
   * Perform menu-specific rebuilding.
   */
  protected function menuLinksRebuild() {
    if ($this->lock->acquire(__FUNCTION__)) {
      $transaction = $this->connection->startTransaction();
      try {
        // Ensure the menu links are up to date.
        $this->menuLinkManager->rebuild();
        // Ignore any database replicas temporarily.
        $this->replicaKillSwitch->trigger();
      }
  }

Because
$this->menuLinkManager->rebuild();
And
$this->replicaKillSwitch->trigger(); (equivalent to db_ignore_replica();)
Are the code that the patch add... and it is already place in 8.8.x branch.

ajits’s picture

ajits
he/him
Primary language Marathi
Location India
commented
Status: Needs work » Needs review

Changing the status as per #47. Maybe this just needs to be closed if the fix is already in?

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

hardik_patel_12’s picture

hardik_patel_12 commented
StatusFileSize
new 2396619-51.patch1.74 KB

Rerolling the patch against 8.9.x-dev branch.

Status: Needs review » Needs work

The last submitted patch, 51: 2396619-51.patch, failed testing. View results

rajandro’s picture

rajandro commented
Issue summary: View changes
Issue tags: -Needs reroll
StatusFileSize
new 8.9.x-dev.png83.33 KB

Removing Needs reroll tag as patch #51 (2396619-51.patch) is working fine with the current core version.
8.9.x

hardik_patel_12’s picture

hardik_patel_12 commented
StatusFileSize
new 2396619-54.patch1.78 KB
new interdiff-51-54.txt509 bytes

Solving failed test cases.

hardik_patel_12’s picture

hardik_patel_12 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 54: 2396619-54.patch, failed testing. View results

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Category: Bug report » Task
Issue tags: +Bug Smash Initiative

This came up in #bugsmash triage. There is no functional bug here, reclassifying as a cleanup task.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Yogesh Sahu’s picture

Yogesh Sahu commented
Status: Needs work » Needs review
StatusFileSize
new 2396619-62.patch1.83 KB

Adding the patch for drupal 9.5.x.
no interdiff because #54 patch fail to apply.

Status: Needs review » Needs work

The last submitted patch, 62: 2396619-62.patch, failed testing. View results

davidmv97’s picture

davidmv97 commented

The patch provided by Yogesh Sahu works on Drupal 9.5 with PHP 8.1 and MariaDB 10.4

sarathkm’s picture

sarathkm
Sapiens
Primary language English
Location Bengaluru, Karnataka
commented
Version: 9.5.x-dev » 10.1.x-dev

Updating the version to 10.1.x dev for reach

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Binoli Lalani made their first commit to this issue’s fork.

Binoli Lalani opened merge request !7939

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Novice

I'd say this isn't really currently in a novice state. Thanks!

Binoli Lalani opened merge request !8046

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.