- Issue created by @m4olivei
- 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
This isn't specific to Navigation but affects everywhere using layout builder, which now will be more places :-D
We had 📌 [Policy] Research about best way to limit the selectable blocks on dashboards Needs review for investigating this. We definitely need a combined approach.
- First commit to issue fork.
- Merge request !10108Issue #3443833: Provide a way for other modules to flag block plugin... → (Open) created by plopesc
- 🇪🇸Spain plopesc Valladolid
Agree that we need a streamlined way to approach this issue, however, it seems that this feature is actually needed by Navigation to help with the integration of other modules. It could be even a Starshot blocker, as described in 📌 Support navigation module Active
Created a MR that provides a hook based approach to declare 'navigation safe' blocks.
Once we have a more robust and global way to declare them in LB, as expected in ✨ Add the notion of a 'configured layout builder block' to solve a number of content-editor and performance pain points Active , the hook could be marked as deprecated and move towards the global solution.
Can we use a third party settings ?
I think it would be easier to use as developer AND as site builder- 🇨🇦Canada m4olivei Grimsby, ON
Couple of observations. First, I noticed that 📌 Support navigation module Active is marked as a Starshot blocker, so you could argue that it should be available for us to use instead of doing this. Although, my read on that issue is that the scope of it is big and it'll be awhile. Not to mention it hasn't had recent activity at all. In that sense, it would be great to see this issue land to unblock other integrations cleanly. The eventual deprecation of this hook in favor of whatever the new approach will be is kind of annoying but should be trivial to do both approaches for a time, so not a huge deal. I'm also not super clear on if the proposal in 📌 Support navigation module Active would cover our use case. The block restrictions piece sounds very site builder focused, backed by config entities. I'm not seeing how you would be able to op-in a set of blocks in code. I posted over there to ask.
All that being said, the MR looks good to me. I've tested it locally and it works just fine with the existing set of blocks (no regressions). List of blocks remains limited to the default set we want to allow.
- 🇨🇦Canada m4olivei Grimsby, ON
Setting it back to Needs Review for now, given open question from @matthieuscarset. Came in while editing my message.
- 🇨🇦Canada m4olivei Grimsby, ON
Can we use a third party settings ?
I think it would be easier to use as developer AND as site builderAren't third_party_settings used for with config entities? In this case we're looking to limit on the basis of block plugin id, not on a specific instance of a block config entity, or other. This is a developer-focused way to signal that their integration has been fully considered for use within the Navigation, given its constraints.
Oh, right
Maybe i mismatched things... i was thinking about adding this as a Condition in Blocks.
So we keep consistency with the rest of the admin UI.
- Merge request !10133Issue #3443833: Provide a way for other modules to flag block plugin... → (Closed) created by plopesc
- 🇪🇸Spain plopesc Valladolid
Created new MR !10103 following instructions provided by @larowlan in Slack to use Block Attribute properties to declare blocks as 'Navigation safe'. For now, using hook_block_alter. Native Attributes could be used once ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active is fixed.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I think the new MR is closer to where we'd want to be - and avoids us adding a new hook/API
The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇺🇸United States phenaproxima Massachusetts
This is a Drupal CMS stable blocker.
Right now we are patching core to get this functionality, but core is a moving target and the patch needs to be rerolled often. This makes it unsafe to ship in a release of Drupal CMS. According to @penyaskito, getting this issue in would fix the problem once and for all. That makes it critically important to land in 11.1 for Drupal CMS.
- 🇪🇸Spain plopesc Valladolid
Updated against 11.x and static analysis flags fixed. Ready for review.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Which MR is to be reviewed?
Hm, the current code in the 10113 MR seems to be moving the logic around but does not actually make it extensible yet? What am I missing?
The other MR adds a new hook and hook_alter.
- 🇺🇸United States phenaproxima Massachusetts
I only just became aware of this issue today, but from chatter in Slack I suspect that the one with the new hooks is NOT the one they are aiming to merge.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Hm how does the MR without the hooks solve the extensibility then?
- 🇪🇸Spain plopesc Valladolid
Hi, magic happens now in hook_block_alter(), according to conversation in Slack (https://drupal.slack.com/archives/C7AB68LJV/p1731064940758699).
Modules can define their blocks adding the property allow in navigation, like in the MR:
$definition['allow_in_navigation'] = TRUE;
This will be moved to block attributes once ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active is merged.
- 🇭🇺Hungary Gábor Hojtsy Hungary
gábor hojtsy → changed the visibility of the branch 3443833-provide-a-way to hidden.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Updated the issue summary with that explanation. I think this makes sense.
Is there a straightforward way to add a test for this? :)
- 🇪🇸Spain plopesc Valladolid
Added tests for the current approach. They will be updated once ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active lands to support attributes.
- 🇭🇺Hungary Gábor Hojtsy Hungary
Thanks a lot! Test looks good to me, I think this is fine :)
- 🇺🇸United States tim.plunkett Philadelphia
If a committer could fix the spacing on the @todo before merge, that'd be cool. But this looks great, thanks!
- 🇨🇦Canada m4olivei Grimsby, ON
RTBC +1 for me!
Worth noting that, while testing this one, I found an existing bug where we don't properly remove all the navigation_menu block derivatives, such that when you are using the Block layout UI, you will see the Navigation specific menu blocks, eg.
To be clear this is not a blocker for this issue, as it was already present in 11.x. I'll file a follow up for that.
- 🇨🇦Canada m4olivei Grimsby, ON
Follow up issue: https://www.drupal.org/project/drupal/issues/3488768 🐛 Menu blocks specific to Navigation are leaking into the Block layout UI Active
- 🇬🇧United Kingdom catch
Committed/pushed to 11.x and cherry-picked to 11.1.x, thanks!
Automatically closed - issue fixed for 2 weeks with no activity.