🇺🇸United States @nicxvan

This is ready for review again.

I took @mstrelan's advice and copied the current system_requirements verbatim to a helper on the Install class.

I then call this with the correct phase.

Since I didn't make any functional changes the baseline only shifted rather than being resolved.

The only changes I made after copying the function:

• Comment line length
• Copied helper function systemAdvisoriesRequirements
• Cast php version to a string for version_compare https://git.drupalcode.org/project/drupal/-/merge_requests/12449/diffs?c...

Any other changes are minor and easy to see in the MR

Ok I regenerated the MR.

I put the existing system requirement on the install class and call that from the hook.

I did not change any functional bits, I only changed references and line lengths.

Since I did not change any functionality I had to regenerate the baseline since there are some baseline issues for system requirements.

I will keep an eye on tests.

It looks like baseline did remove a couple of extra items that were not touched here. I suspect it's just been a while since a full baseline has been regenerated with new rules.

nicxvan → changed the visibility of the branch 3493718-split-requirements-minimum to hidden.

nicxvan → created an issue.

I think this is a partial duplicate https://www.drupal.org/project/drupal/issues/937814 📌 [meta] Smarter UI/API separation for modules Active

Also recipes might solve this.

Sorry didn't read last comment

Just to make sure I understand this only replaced the instances where the error handler is swapped only to find the current handler.

I still have to pull down and confirm it's complete.

Do we need to do a dependency evaluation since this adds the polyfill.

Or is it different since this is symfony?

This code doesn't exist anymore as far as I can tell.

Is there a reason not to use xxh3?

https://xxhash.com/

It seems to be supported and faster, but I'll admit I'm not super familiar with this so I may be missing something.

Ah core services needs to be updated too.

Applied some suggestions for cs and stan.

nicxvan → made their first commit to this issue’s fork.

I think this would prevent hitting the linked issue, but I think this is addressing the symptom not the cause.

I still think the root cause is using private in the first place.

Personally I think we should avoid private just like final.

It closes down people extending classes, we have other ways to mention if something should not be extended for bc purposes.

Also they are just strings, if we wanted to we could just use the strings directly.

Further the whole class is internal so we can change implementation details right?

Ran test only to confirm and it fails with a relevant error.

I think for this issue we would manually run asql command to create a db view on the test db.

Creating an api for creating and managing db views is it if scope here and not necessary.

I don't know how we go about doing my recommendation here, just giving thoughts on approach and scope.

Comments are much clear now.

I have two real questions.

Went through this carefully, looks the same as the previous rtbc besides updating the deprecation.

One suggestion on the deprecation and one question on the callback.

Did you accidentally remove the actual change? I only see the test stub.

My two cents is if we are worried about people resulting in it we should have just used the array value.

If we want them to be constants for consistency then they should be marked internal.

Just because we don't know why someone would want to use it doesn't mean we should preclude it.

This issue is an example of why, if they had not been private I don't think this issue would have existed in the first place right?

Can you convert this to an MR?

Tests don't run against patches.

Ok I've read through this whole issue, there is a lot of great information.

For all intents and purposes the ui module split is well entrenched both in core and contrib and as mentioned there is no real way to enforce this.

As I was reading I was thinking recipes solves a lot of these issues. Also project browser will help with discovery.

I think there are still two big things missing even with recipes:

1. You have to discover the recipe (which also requires someone building them)
2. The module page, how do we organize it in a way that makes these groupings easier.

Is the solution a combination of recipes and a ui for browsing them so that the module page is only for advanced users like drush?

I might share this with the project browser initiative to see if this is being considered.

Took a pass at credit.

I addressed all feedback and created the follow up.

nicxvan → created an issue.

I did it from my phone lol.

I created a new branch from this one too trigger a fresh pipeline to see if that works.

Unfortunately I had typos in the title lol.

I see that too, I am also logged in. Might be worth asking in infra.

Also do we have a link to the other constants where they got added? I am only familiar with the registry ones but there are other constants in the mr.

The one that Alex posted is the test that was failing, this has not been rebased so it won't pass tests.

That failure might be real, I have rerun it 3 or 4 times.

https://git.drupalcode.org/project/drupal/-/merge_requests/11796#note_50...

This is where it was discussed.

Just noting @larowlan wanted them private originally, just curious if we should re evaluate here or if self is enough.

Is it better to just remove private?

Created the mr so I could review.

Since we're in agreement I've removed both todos and updated the metadata.

I'll keep an eye on random fails.

Yeah there was some shuffling of when config is installed.

I think doing the config operations in that hook is generally iffy even if it worked previously.

I pinned @alexpott since I know he worked in this a fair bit.

Why wouldn't we do this with hook requirements?

Is this just for convenience?

I think this belongs in extension, not modulehandler.

Can you share how this would be used?

I've only seen this on the module form list.

The code mentioned in the issue summary has been updated.

Further, I can't imagine not showing a hidden dependency, that would be a huge point of confusion.

There is a todo pointing to this issue so assuming we don't do this at need to remove that.

Postponing on 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active

Made an attempt to update the title. Please update it if that's not accurate.

It's there any reason you've not updated to Drupal 10 instead of looking at 11? It's still a fully supported version of Drupal and will be until December of next year.

Also, to answer your question about what you can do. You can test patches that provide support for Drupal 11, for example the module you mentioned there is an issue here: 📌 Automated Drupal 11 compatibility fixes for ckeditor_div_manager Needs review this issue has been open for over a year and only just started getting reviews in March.

If a module has not updated you can apply for maintainership and provide releases helping the whole community.

That being said I think you can add div with classes natively in ckeditor 5 but please don't quote me I'm not super familiar with that plugin.

Sorry, should be needs work for the reroll.

This is going to be a rebase and the deprecation needs to be updated to 11.3.0.

I don't think this counts as a disruptive deprecation so I assume 12.0.0 is fine for removal.

Copying the relevant cr https://www.drupal.org/node/3459876 →

This update should be 11201

This was the intended replacement.

Confirmed by finding and reviewing the original issue.

I don't think this needs a test.

Not 100% sure on critical, but it seems appropriate.

Yes it should call buildViewsData

https://www.drupal.org/project/drupal/issues/3489415 📌 Deprecate views_field_default_views_data Active is the issue where this was added.

I think we updated the method name but forgot the deprecation: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/datet...

That and the comment probably needs to be updated to call the method in the helper.

Now that 11.2 is out maybe we can get this in.

These two marked obsolete can be deleted:
https://www.drupal.org/node/3499788 →
https://www.drupal.org/node/3499495 →

I've reviewed the other two and all relevant information is there.

xjm → credited nicxvan → .

This is a duplicate: https://www.drupal.org/project/drupal/issues/2621630 ✨ Make Search Field for Module Install/Uninstall usable Postponed: needs info

Consensus seems to be this is an accessibility gate break and should be closed.

Actual postponement and chiming in to say I don't think we should actually do this.

We need compelling reasons to add more attributes after seeing how complex extending them were.

Do we still want this? For example breakpoint had a single check at this point.

If we have the extension object then there is a type.

This has been postponed for 10 years.

I'm not sure this is worth doing, I'll leave this pmni for a few months in case someone wants to revive this.

Postponing on getting the event system removed.

I also think we shouldn't do this, it just adds another layer of an arms race.

The whole hook system has since changed.

Postponing on https://www.drupal.org/project/drupal/issues/3481903 ✨ Support hooks (Hook attribute) in components Active

I don't see anyway to do this now that we don't have hook hook info.

I think invocation time is the way to do it anyway.

Closing this as outdated.

I think we're safe to close this now.

There are some significant complications that prevent this and simple organizational ways to achieve similar organizational goals.

This is how hooks worked in procedural land.

We intentionally preserved that with attributes (though it's under discussion)

I would presume we'd preserve that as well if these mive to oop too.

This might not belong with the extension system, but sure exactly where it should be though.

I think this is related: https://www.drupal.org/project/drupal/issues/3053832 📌 Refactor ModuleListForm Needs work in fact they might be duplicates.

I had moved the other one to extension api, but now I am second guessing, should this be in the extension system?

Should there be a separate issue to move the form?

Gonna officially postpone this.

I'm not sure this status is correct, but I'm not convinced this is the right way to go. There is still some discussion in the impermeable list issue.

This whole system has been replaced.

I was more concerned about how long we'd need to handle the missing config.

After looking at the actual code it's not really a huge concern.

The issue with this is how often do these blocks get resaved?

This is a duplicate, thanks!

I'm not sure .info keys are the right place for this.

If a module really needs to care about this the various requirements hooks give a way to manage already.

That looks great now!

I think we definitely want to decouple from the event system first.

We also have another way to manage the situation that prompted you to create this issue.

I have to look at adding a second compiler pass just for this.

Done! Thanks