Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The return statement is missing in MenuLinkManager::menuNameInUse , in /core/lib/Drupal/Core/Menu/MenuLinkManager.php
public function menuNameInUse($menu_name) {
$this->treeStorage->menuNameInUse($menu_name);
}
The function menuNameInUse in core/lib/Drupal/Core/Menu/MenuTreeStorage.php will always return true
public function menuNameInUse($menu_name) {
$query = $this->connection->select($this->table, NULL, $this->options);
$query->addField($this->table, 'mlid');
$query->condition('menu_name', $menu_name);
$query->range(0, 1);
return (bool) $this->safeExecuteSelect($query);
}
Proposed resolution
The fix would be just adding 'return'
public function menuNameInUse($menu_name) {
return $this->treeStorage->menuNameInUse($menu_name);
}
Remaining tasks
Waiting for review
API changes
core/lib/Drupal/Core/Menu/MenuLinkManager.php method menuNameInUse will return bool.
core/lib/Drupal/Core/Menu/MenuTreeStorage.php method menuNameInUse will return bool.
Data model changes
NA
Release notes snippet
NA
| Comment | File | Size | Author |
|---|
Issue fork drupal-2736647
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:
changes
Comments
Comment #2
ashrafaaref CreditAttribution: ashrafaaref commentedComment #3
kapil.ropalekar CreditAttribution: kapil.ropalekar as a volunteer and at Cybage Software Pvt Ltd. for Cybage Software Pvt Ltd. commentedCreated patch file for the same. Please review.
Thanks !
Comment #4
naveenvalechaComment #5
dawehnerGood catch! It would be nice to have some form of test coverage
Comment #6
kapil.ropalekar CreditAttribution: kapil.ropalekar as a volunteer and at Cybage Software Pvt Ltd. for Cybage Software Pvt Ltd. commentedAttaching patch again. Test with drupal 8.1.x-dev
Comment #8
kapil.ropalekar CreditAttribution: kapil.ropalekar as a volunteer and at Cybage Software Pvt Ltd. for Cybage Software Pvt Ltd. commentedUpdated file path in patch file.
Comment #9
kapil.ropalekar CreditAttribution: kapil.ropalekar as a volunteer and at Cybage Software Pvt Ltd. for Cybage Software Pvt Ltd. commentedComment #11
kapil.ropalekar CreditAttribution: kapil.ropalekar as a volunteer and at Cybage Software Pvt Ltd. for Cybage Software Pvt Ltd. commentedComment #12
kapil.ropalekar CreditAttribution: kapil.ropalekar as a volunteer and at Cybage Software Pvt Ltd. for Cybage Software Pvt Ltd. commentedComment #13
ashrafaaref CreditAttribution: ashrafaaref commentedComment #14
ashrafaaref CreditAttribution: ashrafaaref commentedComment #16
naveenvalechaComment #17
naveenvalechaComment #18
dawehner@naveenvalecha
You are adding quite some stuff, just for putting a return statement in :)
Comment #20
naveenvalechawrong patch :(
Comment #21
naveenvalechaComment #25
ashrafaaref CreditAttribution: ashrafaaref commentedComment #26
ashrafaaref CreditAttribution: ashrafaaref commentedComment #27
ashrafaaref CreditAttribution: ashrafaaref commentedComment #28
ashrafaaref CreditAttribution: ashrafaaref commentedComment #29
ashrafaaref CreditAttribution: ashrafaaref commentedComment #30
ashrafaaref CreditAttribution: ashrafaaref commentedComment #31
ashrafaaref CreditAttribution: ashrafaaref commentedComment #32
ashrafaaref CreditAttribution: ashrafaaref commentedComment #35
ashrafaaref CreditAttribution: ashrafaaref commentedadding the return statement showed up another issue in MenuTreeStorage which I had to fix to pass the tests. hope that helps.
Comment #36
dawehnerGiven this bugfix we certainly needs some tests here ...
Comment #47
DanielVezaMight be playing Devil's advocate here a little, but should we fix this or should we deprecate and remove this? It's not being used by core at the moment and looks like it never would have worked, so isn't this just dead code?
Comment #48
phthlaap CreditAttribution: phthlaap as a volunteer commentedHello, I've uploaded new patch that includes tests. Please help to review.
Comment #49
phthlaap CreditAttribution: phthlaap as a volunteer commentedComment #50
phthlaap CreditAttribution: phthlaap as a volunteer commentedComment #51
phthlaap CreditAttribution: phthlaap as a volunteer commentedComment #52
phthlaap CreditAttribution: phthlaap as a volunteer commentedComment #54
phthlaap CreditAttribution: phthlaap as a volunteer commentedAdded merge request
Updated issue summary with the correct template
Comment #55
phthlaap CreditAttribution: phthlaap as a volunteer commentedComment #56
smustgrave CreditAttribution: smustgrave at Mobomo commentedRan test-only feature
So removing tests tag.
Left a comment on the MR though.
Comment #57
smustgrave CreditAttribution: smustgrave at Mobomo commentedGoing to mark it but do wonder about BC.
Comment #58
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI think we should update the title/IS in case we are fixing another issue/function here (see: #2999314: the service "menu.tree_storage" bug).
Comment #59
phthlaap CreditAttribution: phthlaap as a volunteer commentedComment #60
phthlaap CreditAttribution: phthlaap as a volunteer commentedComment #61
smustgrave CreditAttribution: smustgrave at Mobomo commentedTitle has been updated and feedback addressed.
Comment #62
DanielVezaIs #47 still valid? If there is still no usages in core and this never worked it feels like we should deprecate and remove it
Comment #63
larowlanI agree with #47 let's deprecate this and remove it in 11.
Comment #64
phthlaap CreditAttribution: phthlaap as a volunteer commentedCan someone suggest the steps to remove it?
Comment #65
DanielVezaI've deprecated this method and added a test for the deprecation.
Draft CR here - https://www.drupal.org/node/3415432
Comment #66
DanielVezaPHPstan failure here now actually - Needs further review