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