Move code from the experimental SDC module to core

Lets add a todo list of the stuff that needs moving.

It doesnt like wide table, sorry.

Thanks for organizing all this as a meta issue!

From reading some of the child issues, it seems like the idea is that each issue will open MRs vs this one branch, not against core itself? So someone will decide to merge MRs into here and then the final commit to core will be a monolithic, giant commit? It’d be great to be clear in this parent meta issue so everyone working on this knows what to expect.

Also, not clear what we need a table for here. 😅 seems like a <dl> would be easier to work with and make more semantic sense. 🤓 Unless there are going to be more columns that don’t yet exist for other things?

Thanks again!
-Derek

Yeah we merge against this feature branch. I will open the branches and mr's to this branch today so we dont need to figure that out later.

THere were more columns in my original table, but that broke the layout of the issue. I will convert it to a definition list when i arrive in the contrib rooms in pittsburgh.

We will also need a plan to move sdc_library_info_build to the corresponding location. Likely close to the code that is firing hook_library_info_build.

Updated issue summary.

Fantastic, thanks! That's much more clear. 😅

Today is packed, then I travel tomorrow, but hopefully I can spare some cycles on some of the child issues next week... 🤞 Following them all now, at least. 😉

Cheers,
-Derek

I've also opened a draft merge request to this issue for each child issue. Mostly because we don't use the default workflow since we are merging those issues towards this issue branch instead of main itself.

📌 Move SDC sdc.module functions to Core namespace Fixed might need splitting, because it is about 2 functions in the module file.

Also i didnt open an issue for the tests, they would also need updating in eather the issues that change them, or centrally. But i think there will be less discussion on those, since they might live in places based on the results of the child issues.

Currently the MR in this issue is not mergable, but that should resolve itself when code has actually changed in the branch. Which is hasn't right now.

Whoops. I was trying to help move 📌 Move isRenderArray from SDC to Drupal\Core\Render\Element Fixed forward, cherry-pick'ed my commit into this branch and pushed. I now realize I believe y'all wanted that in MR form, not a direct push. 😬 Should I revert, or do y'all wanna just leave it? It was already RTBC when it was a stand-alone commit. Sorry!

@dww first one is in. Thanks for moving it! Let's keep rolling them in!

e0ipso → credited plopesc → .

📌 Move SDC *.schema.json to Core namespace Active was merged. Creditting @plopesc for that part.

📌 Move SDC Component namespace to Core namespace Postponed is merged.

📌 Move SDC ComponentNegotiator to Core namespace Needs review is also done. Thanks to @plopesc again!

e0ipso → closed merge request !5615

I just re-rolled 11.x. Likely we won't need to do this again since this issue is the last blocker for stable.

plopesc → changed the visibility of the branch 3352256-meta-move-code-merge to hidden.

📌 Move SDC Exceptions to Core namespace Fixed made it to this branch.

📌 Move SDC Discovery namespace to Core namespace Fixed was pretty uncontroversial.

We have 📌 Move SDC Component to Core namespace Active now!

Merged 📌 Move SDC sdc.module functions to Core namespace Fixed . One step closer.

📌 [SDC] Catch missing imports and deprecation messages Fixed was the next in line to complete.

Added reference to 📌 Remove SDC deprecated twig functions before 11.0.0 Postponed

📌 Move SDC Twig\TwigExtension to Core namespace Active is in! So close!

📌 Move SDC Component to Core namespace Active is now merged here.

Only tests are missing now.

e0ipso → closed merge request !5885

Tests are green and all the initial tasks are done. I think it's time for a review.

I scaffolded https://www.drupal.org/node/3410260 → so we can have a URL to reference in the deprecation messages.

@e0ipso if the entire module is deprecated, then the code doesn't need to be deprecated too as such - people will know they need to uninstall it.

I think I got the class_alias wrong. I believe I need to change the alias direction.

I also see this error:

Drupal\Core\Extension\InfoParserException: Extension Single Directory Components (core/modules/sdc/sdc.info.yml) has 'lifecycle: deprecated' but is missing a 'lifecycle_link' entry. in Drupal\Core\Extension\InfoParserDynamic->parse() (line 95 of core/lib/Drupal/Core/Extension/InfoParserDynamic.php).

Some tests are failing. Investigating.

I forgot to remove the dependency of Umami on SDC. Let's see if tests pass now.

Tests are green now. This is finally ready for review.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Merged 11.x into this branch.

The IS has this

Once we have all the smaller issues merged, we will work to clear the release manager and commit manager tags from the big MR,

But what is 'the big MR'?

@quietone we created a feature branch for this issue. We then opened MRs to this issue's branch to start contributing code in smaller bits. This allowed us to have a more manageable scope for peer review and for comments. All the code accumulated in this branch, as soon as we thought it was ready.

Now this issue's branch contains all the code, it is the big MR against 11.x.

I hope that makes sense.

Given that Mateu, Stephen and myself have been reviewing all the individual parts that make up this MR, I feel confident in setting this to RTBC.

Thank you!

The MR pipeline failed three weeks ago, I just re-ran it and it failed again.

I just re-rolled it. Let's see if that fixes it.

This commit above adapts some of the code after 📌 \Drupal\Core\Asset\LibraryDiscoveryParser should use FileCache to cache the parsed yaml Active .

Let's see if we get a green.

Once the MR has been rebased and the merge conflicts solved, I think it is OK to move it back to RTBC.

Thank you!

After applying the MR there are a number of references to sdc remaining, added some comments to see if we can reduce/remove these.

@longwave I addressed your comments in the MR.

@longwave are you good to move it back to RTBC?

Added another round of questions/feedback to the MR.

@longwave most of those comments belong to code that has been moved, rather than changed. This MR is scoped only to moving the code that was approved in the experimental module to its final home. There will be minor changes in code to ensure BC, and some new code in the LibraryDiscoveryParser, but that should be it.

Even if the diff is very big, the changes are not that big.

That being said, I am interested in pursuing the conversations you started but I fear that might derail the scope of this big-enough MR. What do you thing is the best way to address your concerns? Would a follow up issue be a good place for it, or would you rather do it all here?

As mentioned in Slack the only three things I am really concerned about here are:

• The library prefix - do we need to consider BC?
• TwigExtension is getting quite large, should the component extension just be a separate service?
• The BC layer for the component validator service is incorrect.

I think the others can wait for followups.

Oh, would also be interested in your thoughts on service naming as well, because all other core services have friendly string names instead of just class names.

The library prefix - do we need to consider BC?

Yes. This is how we tried to handle it. This section of the code generates the libraries with the old names for backwards compatibility reasons.

I see that there is a small error with the wording of the deprecation message that I will fix right away.

The relevant conversations can be found in 📌 Move SDC sdc.module functions to Core namespace Fixed and in its corresponding MR.

TwigExtension is getting quite large, should the component extension just be a separate service?

Yeah. We touched on that during our conversations while moving it. This is the comment from @plopesc:

Creating a new twig extension would be much simpler, but in the long term, I believe that will be better to have a single one.
I'll work on the BC approach for now.

Do you think that reverting the changes to TwigExtension and moving the SDC Twig extension to Drupal\Core\Template\ComponentsTwigExtension is a better approach? I felt good about both options back in the day, so I'll be happy to implement it.

The relevant conversations can be found in 📌 Move SDC Twig\TwigExtension to Core namespace Active , and its corresponding MR.

The BC layer for the component validator service is incorrect.

You are correct, I will push a fix right away.

@longwave thank you so much for your rounds of review. I know this is a big MR, with lots of context, and most of it is hidden behind children issues. I tried to organize it in sub-issues to facilitate development and review, it turns out it's been more difficult for everyone involved. Sorry.

Throwing something at the CI before logging off. I will finish applying the feedback in the morning.

Oh, would also be interested in your thoughts on service naming as well, because all other core services have friendly string names instead of just class names.

This is something suggested by @larowlan in the MR that got SDC in core as experimental.

Setting back to Needs Review.

Thanks for fixing the BC layer in the constructor.

I added a question about the library BC layer - not sure strtr() does what you expect here, as it replaces bytes in a string?

Going to ask the framework managers for input on:

• Whether to have one combined TwigExtension class or keep the components TwigExtension as a separate service
• Whether to add class aliases or if we are OK just to name services with their FQCN

@longwave I fixed the library bc layer and added a test for it.

Tagging for the framework managers to answer #93 but other than that I think we are done here, thank you for all your work on getting this in!

Eventually we're likely to remove service aliases for things where we don't expect anyone to interact with the service name, but not sure why we're setting a precedent here. On the other hand, is it worth adding the aliases only to remove them later. Don't feel strongly about this to be honest.

I'm confused about what's going on with the TwigExtension - it looks like there's now a dedicated component twig extension, but there are still two additional arguments for the main twig extension, was the separate twig extension already created and those are just cruft, or am I missing something? Looking at the long list of arguments to TwigExtension, I think keeping two separate classes makes sense, but then why would we be adding extra dependencies to the main one?

Many tests are failing.

Sorry @longwave it seems the suppression file had a syntax issue. It is fixed now.

@catch you are correct, that is some cruft from the other day when I split that service in two. I will fix now.

Just wanted to raise a point for Framework Managers about the $schema URL in the components definition.

What we expect folks to add to their metadata yml file is $schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/v1/metadata.schema.json for better DX so that IDEs can provide autocompletion.

I'm not sure pointing to the repo HEAD is the best thing. We can change the schema version by adding a v2 folder and updating the URL, keeping all the versions all the time in the repo.

what I'm not sure is if this URL is going to last 10 years, and if having all the different versions in HEAD is what we want, sounds like it could be somewhere else and not tied to the source of Drupal. Then again it makes sense to have it in the source since that's where all the stuff is defined to begin with.

I don't have a good answer, just questions but I don't think they should hold up this patch.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

I committed a couple of umami components, please update their $schema url as part of this MR :)

📌 Create new SDC component for Umami Banner RTBC
✨ Create header component for Umami, with JS Fixed

Pipeline is back to green.

I resolved merge conflicts and addressed @nod_'s request in #102.

Feedback has been addressed and tests are green.

I think we can move it back to RTBC.

Added some questions about the schema URLs.

Added some questions about the schema URLs.

Yeah. I have addressed that. Sorry for the hasty change, I keep finding myself out of context for this issue every time I work on it. Thanks for catching it. Schema URLs are not super important but we got to get them right.

I've read over the MR multiple times now and there are only really minor nits remaining that ultimately don't matter and could be fixed in followups if needed, so this looks ready to go to me now.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

merged and tests still passing

Nice work folks, left a couple of items on the MR, mostly minor stuff

Review feedback implemented. Everything is back to green. Setting back to NR.

Oh no, I might have been looking at the wrong thing. I see an issue about deprecation messages. Investigating.

Review feedback implemented. Everything is back to green. Setting back to NR.

[déjà vu]

I retitled the CR as it sounded previously like we were deprecating SDC if you did not read it carefully. All the recent changes look good so back to RTBC.

Resolved merge conflicts and fixed an issue number to point the the CR.

Looks like one item still open in the MR

Moving back to RTBC.

larowlan → closed merge request !4147

Committed to 11.x and backported to 10.3.x

Published change record

🎉 Congratulations - huge effort

Added a release note snippet

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

agentrickard → changed the visibility of the branch 11.x to hidden.

agentrickard → changed the visibility of the branch 11.x to hidden.