- Issue created by @hooroomoo
- 🇺🇸United States tim.plunkett Philadelphia
Interesting that node has
\Drupal\node\Routing\RouteSubscriber
, which swaps out the controller and requirements.\Drupal\system\Controller\SystemController::systemAdminMenuBlockPage()
was added in Drupal 5(!) assystem_admin_menu_block_page()
\Drupal\system\Controller\SystemController::overview()
was added in Drupal 7 assystem_admin_config_page()
, and was originally added only to represent/admin/config
. It was reused for/admin/content
in #2085571: admin/content should not depend on node.module → - 🇺🇸United States tim.plunkett Philadelphia
Both controllers use
\Drupal\system\SystemManager::getAdminBlock()
to display links, but differently:SystemController::overview()
iterates over the links 1 level down, and usesgetAdminBlock()
to retrieve the links 2 levels down. It displays all of the blocks in a grid.SystemController::systemAdminMenuBlockPage()
usesgetAdminBlock()
to retrieve the links 1 level down. Since there is only one "block" retrieved, it displays those directly, no grid.I took screenshots of
/admin/config
and/admin/content
with both of the possible controllers.
Note that I had to uninstall the node module first, thanks to the route subscriber mentioned in #3. This makes all of this a lot more theoretical, since keeping/admin/content
in the admin menu while the node module is uninstalled seems like a dramatic edgecase./admin/config
withSystemController::overview()
/admin/config
withSystemController::systemAdminMenuBlockPage()
/admin/content
withSystemController::overview()
/admin/content
withSystemController::systemAdminMenuBlockPage()
- Merge request !4608Issue #3381929: Resolve differences between SystemController::overview and systemAdminMenuBlockPage → (Open) created by tim.plunkett
- last update
10 months ago 30,047 pass - Status changed to Needs review
10 months ago 8:24pm 18 August 2023 - Status changed to Needs work
10 months ago 1:24pm 21 August 2023 - 🇺🇸United States smustgrave
MR looks good
Moving to NW so \Drupal\system\Controller\SystemController::overview can be removed or deprecated.
Assuming a CR will also be needed for the change to the controller and removal of \Drupal\system\Controller\SystemController::overview
- Assigned to tim.plunkett
- Status changed to Postponed
10 months ago 7:51pm 21 August 2023 - 🇺🇸United States tim.plunkett Philadelphia
No, there's nothing wrong with overview. It's just confusing. Hence the added docs in the MR. We're just switching it from 2 usages to 1 usage.
But this might not even be needed, postponing on 🐛 Restrict access to empty top level administration pages Fixed to see how that shakes out
- Status changed to Needs review
10 months ago 2:32pm 30 August 2023 - 🇺🇸United States tedbow Ithaca, NY, USA
🐛 Restrict access to empty top level administration pages Fixed is in.
So noticed that
/admin/config
uses\Drupal\system\Controller\SystemController::overview
you would still see "you have no administrative items." So this still has the problem that #296693 was solving. We just didn't notice because// As menu_test adds a menu link under config. $this->assertMenuItemRoutesAccess(200, 'admin/config');
- last update
10 months ago Build Successful - Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
10 months ago Not currently mergeable. - @tedbow opened merge request.
- Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
10 months ago Not currently mergeable. - last update
10 months ago 30,096 pass, 2 fail - last update
10 months ago 30,098 pass - last update
10 months ago 30,100 pass - last update
10 months ago 30,100 pass - Status changed to Needs work
9 months ago 6:15pm 19 September 2023 - 🇬🇧United Kingdom joachim
The Steps to reproduce in the IS look weird and unrelated to the issue?
- 🇺🇸United States tim.plunkett Philadelphia
I can see why it seems that way, but we found these discrepancies when dealing with *empty* admin pages, and to do so you'd need to remove a bunch of the admin items
- last update
9 months ago Custom Commands Failed - 🇮🇳India kunal.sachdev
kunal.sachdev → made their first commit to this issue’s fork.
- last update
8 months ago Custom Commands Failed - 🇮🇳India yash.rode pune
yash.rode → made their first commit to this issue’s fork.
- 🇮🇳India yash.rode pune
With the fix ^ we can now debug the code in
\Drupal\system\Access\SystemAdminMenuBlockAccessCheck
. Can someone tell the next steps as it is not clear from the current IS or any comment. Thanks in advance. - 🇺🇸United States tedbow Ithaca, NY, USA
See my comment in the MR. I did some work on the tests. I think the tests pass correctly now but somehow should double check. Also we should think if we are missing any test cases.
- 🇮🇳India yash.rode pune
The test coverage looks adequate and
assertUserRoutesAccess()
is giving quite easy to understand error messages. - 🇺🇸United States tedbow Ithaca, NY, USA
Commented on the MR. Here is patch that proves our tests pass with no concern for level checking which should be needed for the differences in expected behavior between overview and system block.
Basically proves our tests are wrong somewhere.
- last update
6 months ago 2 pass - 🇮🇳India yash.rode pune
Hi, after spending some time on this, I think tests on #19 are passing because of https://www.drupal.org/project/drupal/issues/296693#comment-15328018 🐛 Restrict access to empty top level administration pages Fixed , and I think we should have not included that in the test only patch.
- Issue was unassigned.
- 🇺🇸United States tim.plunkett Philadelphia
Running the 'tests-only' pipeline shows a fail as expected. Keeping NW for the IS update
- Status changed to Needs review
6 months ago 9:55am 22 December 2023 - Status changed to Needs work
6 months ago 7:48am 26 December 2023 - Status changed to Needs review
6 months ago 11:15am 26 December 2023 - Status changed to RTBC
6 months ago 9:34am 27 December 2023 - last update
6 months ago 2 pass - last update
6 months ago 2 pass - last update
6 months ago 2 pass - last update
6 months ago 2 pass - last update
6 months ago 2 pass - last update
6 months ago 2 pass - last update
6 months ago 2 pass - 🇧🇪Belgium Jelle_S Antwerp, Belgium
FYI: This introduced a bug. See 🐛 Admin page access denied even when access is given to child items RTBC
- 🇨🇦Canada phjou Vancouver/Paris 🇨🇦 🇪🇺
The patch seem to fix the regression introduced with the Drupal core update with 🐛 Restrict access to empty top level administration pages Fixed
- First commit to issue fork.
- 🇳🇿New Zealand quietone New Zealand
I'm triaging RTBC issues → . I read the IS, the comments and the MR. I don't see any unanswered questions.
This is tagged for an issue summary update which was completed in #22, so removing tag.
Adding tag for a follow up for this comment, https://git.drupalcode.org/project/drupal/-/merge_requests/4608#note_242911
When reading the MR (not a review) I made suggestions for several comments. Since they are minor I decided to go back and apply them, tests are running now.
- 🇳🇿New Zealand quietone New Zealand
The tests have passed and they were minor changes so I am leaving this at RTBC.
- First commit to issue fork.
- Status changed to Fixed
4 months ago 12:53pm 26 February 2024 - 🇬🇧United Kingdom catch
Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks! Enough change here not to try to backport to 10.2.x but sounds like this doesn't come up often in practice. I made and accepted a couple of comment suggestions on the MR prior to commit.
Created the follow-up at 📌 Use 'level' instead of 'child' 'grandchild' 'great grandchild' in menu_test and associated tests Active
Automatically closed - issue fixed for 2 weeks with no activity.