Create enums for RequirementSeverity and RequirementPhase

Created a MR, but I know there will be test fails. I think the biggest issue is going to be BC support and converting from int values stored in config.

Remaining tasks:

• Handle BC for contrib modules that will still be using constants.

I think this issue is big enough without trying to introduce enums for RequirementPhase at the same time.

Also adding that we are deprecating drupal_requirements_severity() and moving this to the enum.

Can we add a simple deprecation test to make sure the current constants are returning a deprecation warning.

Yeah, I think we need tests to make sure the BC layer works when using those constants.

Added a bc test for getMaxSeverity(). There are a few instances where we check the severity status.

BC tests added look great! Moving to NW for the CR.

Updated CR

Thanks! CR reads well.

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.

Rebase on 11.x

Posted a lengthy review.

    There was 1 error:
    
    1)
    Drupal\Tests\system\Functional\UpdateSystem\UpdateScriptTest::testRequirements
    Behat\Mink\Exception\ResponseTextException: The text "This is a
    requirements warning provided by the update_script_test module." was not
    found anywhere in the text of the current page.
    
    /builds/issue/drupal-3410938/vendor/behat/mink/src/WebAssert.php:907
    /builds/issue/drupal-3410938/vendor/behat/mink/src/WebAssert.php:293
    /builds/issue/drupal-3410938/core/tests/Drupal/Tests/WebAssert.php:956
    /builds/issue/drupal-3410938/core/modules/system/tests/src/Functional/UpdateSystem/UpdateScriptTest.php:150
    /builds/issue/drupal-3410938/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

Added further feedback, tests still failing.

Yeah, would be great to do this and 📌 Convert hook requirements that do not interact with install time Active at the same time, so that the new hooks only use the new API.

kim.pepper → changed the visibility of the branch 11.x to hidden.

Looks like \Drupal\package_manager\ValidationResult was created using int values in ✨ Add Alpha level Experimental Package Manager module Needs review before this could get commited. 😞 Created a follow up 📌 [PP-1] ValidationResult should use RequirementSeverity enum Active

Feedback has been resolved and all tests are green 🟢. Back to NR.

I've only reviewed this at a high level, it looks good on the surface, I'd have to do a deeper review to mark this ready.

Honestly my only concern is I think 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active should get in before this one, it's FAR easier to rebase this issue and update than to rebase the other issue since the other issue is a lift and shift. We'd have to replicate the change.

I'm not going to go as far as postponing this in case I'm missing something.
But that is the last of two conversions. The other being a test module that is trivial to redo.

Had some time to think about this, actually this should definitely take precedence. Since it's a deprecation.

I did see a couple of documentation nitpicks that I could see in a follow up (the ones that only mention hook_requirements)

There are some comments mentioning the constants that I think need to be updated and one level that changed I had a question on.

Putting it back to needs work, feel free to ping me on slack once it's updated.

Thanks for the reviews. Resolved all threads.

I think this is ready!
I took a good look, this is a good step towards cleaning up install time and requirements checking.

I pulled the code down and did a search, the only remaining instances are in tests for legacy and BC considerations.

I read the comments and found some things that should be changed.

Also, I can't find confirmation that the change record has been reviewed. Can someone check that?

Forgot to mention I did review the CR and made some updates.

FYI the docs say quotes for array keys in array shapes aren't needed but I added them anyway https://phpstan.org/writing-php-code/phpdoc-types#array-shapes

@kim.pepper, thanks for pointing that out. I didn't expect to see that at the end of list. I would have written it differently.

This mostly looks great, thanks so much!

FYI: working on an MR review. It's a big diff, so it's taking a while. 😅 I found some stuff worth fixing before commit. Wanted to update status now to get this out of the RTBC queue. Stay tuned...

Overall, this is a big improvement!

As promised, opened a bunch of MR threads. Many are nits, but I'm concerned we're linking to the wrong CR in some places, not convinced about the namespace for the enum class, and a few other loose ends.

Per MR review, tagging for CR updates to mention `StatusReport ::getSeverities()` is going away.

Initial pass at saving credit. So far, looks like everyone has meaningfully contributed. 🎉

Updated the CR about StatusReport ::getSeverities():
https://www.drupal.org/node/3410939/revisions/view/14001271/14002566 →

Pushed commits to resolve most of the threads I opened.
Created 📌 [pp-1] Remove loadInclude and @todo from Update Status after #3410938 is in Active for 2 of them.
Left 3 still open for feedback from @kim.pepper or other reviewers.
Pipeline is green again. Back to NR.

Thanks!
-Derek

Discussed changes in slack and reviewed all of the updates.

There are a couple of open threads as mentioned, but I think they are adequately addressed.

I asked @dww to create the follow up mentioned.

Putting back in RTBC!

Fleshing out API changes section of summary.

Committed to 11.x and backported to 11.2.x

Published change record

Thanks all!

larowlan → closed merge request !5958

Created follow-ups:

• 📌 deprecate drupal_requirements_url() and move to a method on RequirementSeverity enum Active
• 📌 Change the requirement array keys to int values in StatusReport::preRenderGroupRequirements() and deprecate RequirementSeverity::status() Active

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

Looks like this broke drush against 11.2, see https://github.com/drush-ops/drush/issues/6298

Not clear to me if this is drush relying on something @internal or whether the bc layer is incomplete re-opening temporarily to figure out whether it's just something to fix in drush or whether we need a core issue to fix the bc break.

We changed what we return in requirements hook. It's not possible to provide BC for that I think. We would have needed to introduce a new array key and keep the old one for BC or something like that, that's a alot of work. I also expect that monitoring and some others that call into requirements hook are at least partially broken with this.

I had the idea that we would only convert and use the new enums in converted hook implementations, those are new hooks anyway and wouldn't be found by existing code anymore (*and* also combine it with conversion to requirement objects, which also didn't land yet)

The problem is that we converted system_requirements() before that was converted to a new runtime/update requirements hook as 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active didn't make it into 11.2.

system_requirements is ready, is it enough to just get that conversion into 11.2.1?

As berdir said I'm not sure how to approach this otherwise.

Discussed this with @xjm - the drush issue was traced to the monitoring module and has been fixed there.

Setting this back to fixed. Will try to get 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active in to 11.x soon but it is too big to consider back-porting.

Oh yeah of course and workout backport it would not fix 11.2 anyway.

Glad we identified the root cause.

I really wanted to come here and say "The committers discussed this and agreed that no, we will not backport the 3.4K LOC diffstat API conversion that affects every site to a patch release", but Lee pre-empted me. 🤣

Hmm, there are 3 reproducible issues in drush that probably had not explicitly mentioned in the GitHub issue but got addressed by the attached PR. It's OK converted to Warning for drush updatedb, It's an exception for drush rq, and another warning when validating enabled modules. That's all above and beyond the issue caused by the monitoring module.

Still, not worth undoing this big change, I guess. The warnings on the console are annoying, the exception is probably very rare as I've never seen anyone using that drush command before. We should be able to work around these 3 issues in a proper drush PR (mine isn't BC) that deals with those 3 issues by running through a few if-statements to get it right. Just for the record here.

Thanks @jurgenhaas for the extra context

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