[No commit] Prepare for core inclusion

Simple start to core inclusion adjustments. This will evolve. Need something to write the script against.

Here is a script adapted from the one @lauriii wrote for Claro → .

The most noteable difference is that this is written to push to the core navigation module issue fork ✨ Add the new Navigation to core as an Experimental module Fixed . I've commented out the final lines to do that for now to allow an opportunity to review this approach. But testing locally shows that it should be ready to go, granted you have push access to the core issue fork.

Other highlights / open questions:

• This script clones navigation module and applies the patch found here in #4. There will be further needs for adjustment that are only relevant to core, so that patch will need to evolve. For instance, the ShortcutsNavigationBlock and UserNavigationBlock should be moved to their respective core modules.
• Just to be clear, we are keeping the navigation name correct?
• Will it be marked as experimental?
• Do we want a better module description? Currently reads as "New administration navigation"
• The script runs yarn prettier, and catches a few things in our JS and CSS. We should probably just clean those up for the contrib module repo as well.

Replaced the core inclusion patch in #4 with a MR: https://git.drupalcode.org/project/navigation/-/merge_requests/238

Also updating the script to pull the diff from the MR. Including an interdiff for that.

Oops, diff'd it the wrong way. Here is a correct interdiff for the script.

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

Apologies for chiming in without any context on the module (yet). I was pinged for ideas, and here I am.

From past experiences I would:

• Define the back-end APIs of the experimental module. Anything that is not an explicit API should be impossible to use as such. Mark dockblocks with @internal, make services private, turn classes or methods into final, make methods private. This will help with making it beta (more on that below).
• Perfect is the enemy of good enough. There is no innovation when aiming for perfection. Get it into core. This means that if there are any lingering issues that tend to get into bikeshedding ask yourself: is this remaining thing critical enough to miss the deadline for 10.3?
• Remember that experimental modules are removed from the codebase before a tag is made. This means that there is a chance that you put this into 10.3.x and you don't see it in 10.3.0, 10.3.1, ... until the experimental module is marked as beta. Create an issue to mark the module beta. You can use 📌 Mark SDC as beta so it can be included in 10.1 Fixed as a reference.
• Start the official documentation. Even if it is just a big TBD. This will give you a place to link things to. This is the SDC one for reference: https://www.drupal.org/docs/develop/theming-drupal/using-single-director... →
• Define the front-end API. I am ignorant on how this works, but I think you should give this some thinking. It is worth showing that you put some thought into it. Things like: What is the markup that themes can rely upon not to change? How will they alter/extend the look & feel? Using SDC would solve this because anything internal to a component is subject to change, the API is the the *.component.yml file, and you alter/extend with the usual component replacement.
• Framework managers, and core committers are the busiest people in the community. Walk the fine line of reminding them that this needs attention, but without pestering (they may have more pressing matters to attend to). They are gifting reviews, try to have everything ready for them before you summon their attention to the MR, as an expression of gratitude for their time.
• On my part, I can commit to a review for the back-end part of the module, once the PR is up. This should make it closer to an RTBC (necessary to get any eyes on this). Plan on having someone to do the review for the front-end part, do not wait for spontaneous reviews (those may not happen). If you don't find a reviewer, try trading reviews with someone.

Thanks @plopesc.

I think we need to mark the module as experimental in the *info.yml file

I'm wondering this as well. I'm not clear on the etiquette, even considering @e0ipso's notes. I thought I remember @ckrina saying at some point we weren't marking it as experimental, but I could be wrong there.

Should changes in *info.yml file be present only in the core's MR but not in the "official" repo?

I commited some simple changes to *.info.yml to match what Claro had prior to their core inclusion patch. I think any changes that can live in the module in contrib land are good to have ther to make the adjustment patch/MR only as big as it needs to be.

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

Good to know. We should get clarity on this point. If our core patch/MR does need to move classes like ShortcutsNavigationBlock, we'll need to carry along a patch here that the script will apply, like Claro did. The MR here on this issue won't be enough, b/c the MR is limited to affecting just the code in the navigation module. Either that or we have a combination of mv / sed commands in the script that does that move.

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.

I like it. Lets get this going.

Being restrict with maintaining all pipelines green before merging parallel work would be great

Agreed.

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.

I forgot to mention. If you are planning to move classes, take a moment to find the correct eventual location. Once done, add class_alias to the current location.

We clarified in our meeting today:

• The module will be marked experimental. That's a change we'll need to make to the MR here.
• Since it's experimental, the shared understanding is that we won't need to move classes yet. Thanks @e0ipso for the note on class_alias.

Also, thanks @e0ipso. Your wisdom is always welcome!

Define the back-end APIs of the experimental module. Anything that is not an explicit API should be impossible to use as such. Mark dockblocks with @internal, make services private, turn classes or methods into final, make methods private. This will help with making it beta (more on that below).

Great point. I've filed an issue for us here: 📌 Review and define the back-end APIs of the experimental module Active .

Remember that experimental modules are removed from the codebase before a tag is made. This means that there is a chance that you put this into 10.3.x and you don't see it in 10.3.0, 10.3.1, ... until the experimental module is marked as beta. Create an issue to mark the module beta. You can use #3354860: Mark SDC as beta so it can be included in 10.1 as a reference.

We discussed this point at the meeting this morning. I think we had some confusion around where the beta marker would be, which I see you had the same question there and got a good response from @alexpott 📌 Mark SDC as beta so it can be included in 10.1 Fixed . To summarize, there is a page here documenting "Experimental modules and themes in Drupal core" → . On that page there is a table of current experimental projects → . The table has a stability marker. Until the stability is beta (and/or at core committer discretion), the module is removed from tagged versions of Drupal core.

We're presently in a beta phase in the contrib project (tagged last week). My impression is we may need additional documentation and/or rationale up against the requirements → . Not totally sure.

Define the front-end API

We should create an issue for this one for review / definition.

On my part, I can commit to a review for the back-end part of the module, once the PR is up. This should make it closer to an RTBC (necessary to get any eyes on this).

Thanks for the offer! We'll definately take you up on it.

Experimental requirements docs: https://www.drupal.org/about/core/policies/core-change-policies/experime... →

We'll need a handful of changes only when navigation is living in the core/modules directory to outside drupal core files. Here is a patch for that. Not really a good place to put this as an MR that I can think of, but open to suggestions.

Here is an updated script that.

• Meant to run on an up to date checkout of Drupal core 11.x
• Will establish the core inclusion MR branch from ✨ Add the new Navigation to core as an Experimental module Fixed on your local
• Will clone the navigation contrib project into core/modules overwriting anything there
• Will apply the MR patch from this issue. These are changes to navigation module source code required for core inclusion.
• Will cleanup the navigation clone, removing unnecessary files and git history
• Will apply the patch from #15 of this issue. These are changes to core files required for core inclusion.
• Will run prettier and build:css to ensure source files meet core standards, although they should already.

After a script run, the changes necessary are all staged for commit. They should be reviewed, committed, and pushed to the core inclusion MR.

Updated patch. The phpstan ignore path was incorrect.

Updated script.

Updated patch and script. Adds some eslint globals.

Thanks! Closing since we're in!! :D

Automatically closed - issue fixed for 2 weeks with no activity.