If I understood correctly, the bug reported is related to the need of creating manually the shortcuts when installing the module in minimal profile.
In that case, I believe the module is working as expected.
Navigation should not show the shortcuts section unless there are actual items for the user. It would be a useless item in the bar with no purpose.
Shortcut links are independent of navigation module, and this module can't take that responsibility, I think.
Minimal profile does not provide default links because it aims to reduce all the boilerplate content. Standard or Umami are more opinionated and generate more content by default.
Feel free to reopen the issue if I misunderstood the summary.
In situations like this, it's helpful to provide the expected behavior or a resolution proposal to have a better understanding of the scenario you find problematic.
Thank you!
Thank you for your review!
I think the scenario described in your manual test is missing the step to create shortcut links.
When there are no shortcut links to show to the user, the shortcuts navigation block is not rendered.
According to https://git.drupalcode.org/project/drupal/-/blob/11.x/core/profiles/stan.... Standard profile creates some shortcut links by default during installation.
It is possible that this is not happening in minimal, where you need to create some shortcut links by yourself to have access to the shortcuts block.
Could you please repeat your tests and confirm this?
Thank you!
Tests added to MR !7944
Added 🐛 Special Menu items are rendered as empty links in navigation Active
During Drupalcon sprints worked on an approach where ID attributes, and all the references to them are removed from the HTML markup before generating the hash, so the problem is gone, but at the cost of some extra calculations that would be great to avoid.
Would be great to have some extra feedback about whether we can get rid of id attributes or should we explore other paths assuming that IDs could be there.
On the other hand, if we remove IDs from the core templates, but a specific site overrides the template making use of them, the navigation would be broken.
That's something that already would happen with current toolbar, but need to confirm if that scenario should be supported by navigation.
We can close this given that 🐛 Regression: Shortcuts menu flickers when the page is loaded Fixed showed the way to avoid flickering with no need to make an extra effort at render array level.
Same approach could be followed in the future if necessary.
Talked to @mherchel in the weekly Drupal Admin UI call and he also agreed.
MR created.
There is an, apparently, unrelated failing test. Will relaunch the test suite later.
Thank you both for taking care of it!
I think we can mark this as RTBC.
Created MR that creates a MenuLinkTree manipulator that triggers an event that gives opportunity to other modules to react to the navigation menu build process.
Created an event subscriber that removes the Help and Content links from admin menu.
Doing it in this way, the original menu is not being altered, and provides an extensible system that allows other modules to interact with the navigation menu link tree.
plopesc → made their first commit to this issue’s fork.
I really like the approach proposed in #7 or a similar one.
However, I don't see where we can connect the Icon plugins with the menu items.
Menu item class for menu items created through yml files are like toolbar-button--icon--system-admin-structure that can be inferred from the menu definition. It would require some Drupal knowledge to discover the specific class. These menu items cannot be modified from the UI though
However, if editors want to add menu items through the UI, icon class is like toolbar-button--icon--menu-link-contenta614ff3e-4306-4830-933c-f152268ea849 which is not friendly and also can differ between the different environments.
.
At this point 2 possible approaches come to my mind:
- Add an extra config property in NavigationMenuBlock where editors can define the menu item - icon relationship manually. Values for core menu items would be shipped by default. For other NavigationBlocks like Shortcut or User, the icon could be chosen from the list of available plugins and stored as part of the block config.
- Modify the menu item form UI to add a field from where each menu item could relate to an icon to be used just in case the menu item is used for navigation. These relationships could be stored as third party settings in the menu config entity. hook_install() would take care of setting the default values when installing navigation. For other NavigationBlocks like Shortcut or User, the icon could be chosen from the list of available plugins and stored as part of the block config.
Would be great to have some other ideas brought to the table and reach some kind of agreement before starting to work on this.
Thank you
As discussed with @ckrina in Portland, this new block will be hidden by now, but used for the Help Navigation item for testing purposes and simplify the footer logic.
Added functional tests to ensure link access rules are preserved.
Attaching screenshot to ensure that there are no visual regressions.
Thank you for working on this one!
Would be great if you could merge 11.x branch into the MR branch to fix the broken tests. They're unrelated to the issue, but to the specific 11.x HEAD you used as base.
While we are here, would be great if you could remove the moduleHandler property from the NavigationUserBlock class.
You can completely remove __construct() & create() methods. We could also get rid of the reference to ContainerFactoryPluginInterface in the class definition.
Thank you!
penyaskito → credited plopesc → .
Working on it!
Postponing due to necessary implementation discussion.
You were right.
Blocks need to be accessible for administrative purposes. Block itself has to ensure the logic in build() is safe enough and not rely on blockAccess() for logic that could throw unexpected exceptions.
Thank you for your help here.
Created 🐛 BlockComponentRenderArray::onBuildRender() does not check block access in Preview mode Active
To try to address the LB preview issue.
If that gets in, this one could be closed.
plopesc → created an issue.
Thank you for your work on this.
The root source of this issue is that in LB managing page, apparently NavigatioShortcutsBlock::blockAccess() method is not being invoked.
That module checks whether shortcuts module is enabled or not before continuing.
Form this point I think we have some alternatives:
- Try to find why blockAccess() is not being invoked and try to invoke it even for the LB config page
- Add an extra check in NavigatioShortcutsBlock::build() to ensure that Shortcut module is enabled
- Continue adding shortcut as a navigation dependency. If so, I would remove all the code checking if Shortcut module is enabled
From my perspective, I would avoid the 3rd option because that would add an unnecessary dependency for the future.
Code looks good and the administrative UX has been improved.
Also provides tests.
Thank you!
Thank you for your efforts here.
Unfortunately, navigation_top_bar is just an instrumental module to prevent access to the experimental top bar unless site administrators assume all the possible risks.
It means that all the code and logic should remain in the navigation module to make the transition form experimental to stable as simple as possible avoiding BC breaks.
Discussion about strategy and implementation can be found at 📌 Create feature flag module to control navigation top bar visibility Fixed .
Marking it as won't fix because moving code to navigation_top_bar module is discouraged.
Selector improvements in the test are nice, but I believe we can introduce them as part of a different top bar related issue, like 🐛 Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active
plopesc → created an issue.
Code updated against 11.x
Closed MR against contrib navigation module. Created equivalent MR against core.
Moving to NR.
Instead of adding a pre_render callback, opted for adding a simple placeholder when user is not allowed to see the navigation bar because it's less intrusive with the LB render array generation process.
Good catch 💯
Added logic to include the font if only the toolbar is being rendered.
About font split. Do you think it has to be addressed here? Or might be moved to a lower priority follow up?
plopesc → created an issue.
Thank you for being able to capture the glitch!
For reference: This flickering was introduced as part of this comment in the main core
MR:https://git.drupalcode.org/project/drupal/-/merge_requests/7474#note_297577
Need to find a way to have a cacheable placeholder.
We could consider this a duplicated of 📌 Agree on a lazy load strategy Active or vice-versa.
Thank you for your reviews.
Navigation already provides a basic hook_requirements() implementation: https://git.drupalcode.org/project/drupal/-/merge_requests/7474/diffs#80...
If you think it can be improved, or is not descriptive enough, we could update it.
Hi @bnjmnm
The reason why it failed is because the --NUMBER suffixes added to the IDs modify the HTML markup, hence the hash generated from that markup is not consistent.
This is because Html::getUniqueId() methods adds a random identifier for AJAX calls
Here are the steps to reproduce it:
- Replace
clean_idwithclean_unique_idin navigation-menu.html.twig - Clear Drupal cache
- Clear browser local storage
- Disable render cache from /admin/config/development/settings or config files
- Reload the page and confirm that there's an AJAX error and navigation menu nested levels are not being loaded.
Getting rid of the id suffix solves the issue in the meantime, even if the HTML markup is not correct. It has to be resolved somehow, though.
Another option would be to get rid of id attributes in the template, but I'm not sure if that's possible for accessibility reasons.
Tests are green and all the MR threads are closed.
Moving to Needs Review again for a new round of reviews.
Thanks to everyone who has worked to make it possible!
This is not an issue anymore, given that Broken navigation plugin was removed as part of the BE refactor made by @larowlan.
Worked on a POC that use client-side cache for the whole navigation tree, while loading only the 1st level from the server.
The approach is a cloned adapted from how Toolbar module works.
My JS skills are limited, so would be great if someone could take a look into them.
Also need to figure out how to avoid duplicated IDs when same block is placed more than once clean_unique_id twig filter does not work well for individual AJAX calls.
My initial thought was to use the block ID, but I found this issue: 💬 Retrieve unique block ID or machine name in build() method of BlockBase class Active
Feedback and ideas are welcome!
Working on it.
Backend part is ready for review. We need now to ensure that Top Bar specific styles are moved to the submodule.
Adding reference to 📌 Create feature flag module to control navigation top bar visibility Fixed
@larowlan Thank you for opening that discussion.
We need at some point to address the code duplication issue.
I'm not sure whether this is a task to work on now or when moving the module from experimental to stable.
If I understood correctly, experimental modules recommendation was to not modify other core pieces just in case they will not be finally promoted to stable. Not sure if that's not actually the case, or if this scenario could be an exception.
Since the arrival of 📌 Use PHP attributes instead of doctrine annotations Fixed adding a new arbitrary property to Block attributes is not allowed.
I'm getting this error when adding the new property to attributes:
Error: Unknown named parameter $allow_in_dashboard in ReflectionAttribute->newInstance() (line 13 of modules/contrib/dashboard/src/Plugin/Block/DashboardTextBlock.php).
Read issues comments and there are some mentions about how to alter them, but most of them leads to use and alter_hook, so defining blocks as dashboard-allowed from the attributes section does not seem to be an option, at least for not.
What do you think?
Thank you for your review @penyaskito
Cleaned up the code a bit and fixed all the code quality and functional tests.
We might need a more in deep review if you consider it appropriate and determine whether we should refactor tests to cover both Block and Layout Builder as part of this issue or as follow-ups.
Thank you!
MR merged. Opened 📌 [PP] Define specific tests for disabled super user access policy Active to implement extra tests for disabled super user access policy once it is added to a stable release.
LGTM!
Good point!
I don't know well how to replicate the user 1 behavior in kernel tests after 📌 Add a container parameter that can remove the special behavior of UID#1 Fixed .
Actually this MR is agnostic and gives the actual responsibility to the site administrator.
If they decide to give superpowers to user 1, they'll access to all the dashboards.
If user 1 is not empowered, it would behave like any other.
Don't think we need to add extra tests to confirm the default behavior.
If you feel safer having them, we could explore how to make it possible.
Yep, I believe we will still to use this workaround - or a different one - for now.
MR ready for review
@m4olivei
There were some unrelated test fails due to 📌 [Meta] Fix all tests that rely on UID1's super user behavior Active . Worked on them here to avoid creating a new issue.
Fixed them, I hope we don't have more unexpected conflicts here.
New MR adding required missing dependencies in default config
Move to NR
I think the contrib module needs to remain usable on the current release of Drupal (both for developers working on it and test run). I think for these test fixes, it's a case of leaving in the contrib module what satisfies the test runs on the current major, and addressing issues in the next major as a change required in #3439747: [No commit] Prepare for core inclusion, in the MR meant to stand in as a patch for the sync script to apply.
Good point!
Worked on issues related to CSpell, PHPCS & PHPStan.
Some notes:
We should add to the core MR modules/navigation/assets to .cspell.json & .eslintignore ignore list
We should add exceptions for CategorizingPluginManagerTrait in .phpstan-baseline.php file
$ignoreErrors[] = [
'message' => '#^Call to method getDefinitions\\(\\) on an unknown class Drupal\\\\Core\\\\Plugin\\\\CategorizingPluginManagerTrait\\.$#',
'count' => 1,
'path' => __DIR__ . '/lib/Drupal/Navigation/NavigationBlockManager.php',
];
$ignoreErrors[] = [
'message' => '#^Call to method getSortedDefinitions\\(\\) on an unknown class Drupal\\\\Core\\\\Plugin\\\\CategorizingPluginManagerTrait\\.$#',
'count' => 1,
'path' => __DIR__ . '/lib/Drupal/Navigation/NavigationBlockManager.php',
];First iteration of the API definition marking some features as internal/final.
Would be great to have a second pair of eyes on this to ensure that I understood it correctly.
Thank you!
jlbellido → credited plopesc → .
Good job @m4olivei!
These is a great starting point for this one.
Some comments/thoughts here related your questions:
- I think we need to mark the module as experimental in the *info.yml file
- Should changes in *info.yml file be present only in the core's MR but not in the "official" repo?
- From what I understand, while the module is in experimental phase, all the classes that could/should be provided by other modules like ShortcutsNavigationBlock should stay in Navigation module. They're moved when the module becomes a first class passenger.
- An internal MR to cleanup all the code quality remaining issues would be necessary before the actual core MR would be necessary because in core MRs are deal breakers and actual PHPUnit tests are not even run if they fail. See last pipeline results.
- Being restrict with maintaining all pipelines green before merging parallel work would be great
Created a initial patch for this that solves the initial issue, but could escape strings twice, like in the failing test.
Tests fails because views provided blocks have the following admin_label:
$admin_label = new TranslatableMarkup('@view: @display', ['@view' => $view->label(), '@display' => $display->display['display_title']]);
That means that malicious values in the view or display names are already escaped in advance.
However, an admin_label where the malicious part is not in a token, like the one below, should be escaped:
new TranslatableMarkup("<script>alert('XSS subject');</script>")
As far as I know, we don't have a reliable way to determine whether a string has been already escaped to determine if a second escape is necessary or not.
In the current scenario, I'm not sure whether it's better to allow XSS unsafe strings, or have double escaped ones for views. Any of the scenarios is unlikely to happen.
If someone has a better approach to solve this situation, any advice is much appreciated.
Thank you for your feedback.
Obsolete state keys are removed as part of the post_update hook.
Created change record draft:
https://www.drupal.org/node/3438802 →
larowlan → credited plopesc → .
Conflicts resolved and tests are green again. Back to NR.
Twig debug and related development toggles from "Development settings" page moved to raw key value storage as part of the MR.
MR created to try to address this shortcut caching improvement.
Thank you!
Sorry for being late to the party!
I would encourage to provide tests for this new feature. They'll probably be required at some point.
MR ready for review.
Part of the test is commented because are related to shortcut module caching issues that are not fixed in 10.2.x
Once tests are run against 10.3.x or 11.x, they'll be necessary.
Branch updated with latest changes from 11.x and tests are green again.
Thank you for the heads up!
Removed the remaining BC layer deprecation notices.
Fixed. Thanks!
Looks great!
MR created.
Active once 🐛 Shortcuts toolbar links are not updated automatically when default shortcut set is changed Needs review is marked as fixed.
Thank you for the heads up.
Brought back the SDC module info file stub and added post_update hook to uninstall it.
Finally added section https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsol... → to reflect the module situation
MR created that replaces the problematic user cache context with user.permissions to make the navigation bar cacheable.
As a secondary effect, we found that cache tags were not being implemented properly because navigation bar was not including automatically new navigation blocks added.
Root cause seemed to be that navigation block list cache tag (config:navigation_block_list) was not included in the bar itself.
Once the cache tag is added, tests are back to green and 'X-Drupal-Dynamic-Cache' header is not 'UNCACHEABLE' anymore.
I believe this is ready for review now!
As part of the MR creation, commented the line suggested by @m4olivei in 📌 Add tests for UserNavigationBlock Needs work .
It lead to some broken tests.
Debugged it a it and found the issue in NavigationBlockCacheTest::testCachePermissions
Cache tags before the change (Test pass):
config:block_list config:navigation.navigation_block.administration_menu config:navigation.navigation_block.content_menu config:navigation.navigation_block.oxaw6wfu config:navigation.navigation_block.user config:navigation.settings config:system.menu.admin http_response navigation_block_view rendered user:2 user_view
Cache tags after the change (Test fails):
config:block_list config:navigation.navigation_block.administration_menu config:navigation.navigation_block.content_menu config:navigation.navigation_block.user config:navigation.settings config:system.menu.admin http_response navigation_block_view rendered user:2 user_view
I'm missing the cache tag related to the test navigation block added during setup().
My feeling is that reason behind failing test is that navigation bar cache tags are not properly invalidated or refreshed when a new navigation block is added.
More digging in this one will be necessary. I'll try to continue with this next if anybody finds the root cause before.