Create enums for RequirementSeverity and RequirementPhase

Created on 26 December 2023, over 1 year ago

Problem/Motivation

Split out from πŸ“Œ Add value objects to represent the return of hook_requirements Needs work we should create enums for RequirementSeverity and RequirementPhase as a first step.

Steps to reproduce

Proposed resolution

Create enums for RequirementSeverity and RequirementPhase.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component
ExtensionΒ  β†’

Last updated about 1 hour ago

No maintainer
Created by

πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @kim.pepper
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia
  • Status changed to Needs review over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    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.

  • Pipeline finished with Failed
    over 1 year ago
    Total: 603s
    #68526
  • Pipeline finished with Canceled
    over 1 year ago
    Total: 574s
    #68541
  • Pipeline finished with Failed
    over 1 year ago
    Total: 515s
    #68546
  • Pipeline finished with Success
    over 1 year ago
    Total: 714s
    #68556
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Remaining tasks:

    • Handle BC for contrib modules that will still be using constants.
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    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.

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

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

  • Pipeline finished with Success
    over 1 year ago
    Total: 635s
    #70838
  • Pipeline finished with Success
    over 1 year ago
    Total: 737s
    #70846
  • Status changed to Needs review over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

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

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • Status changed to Needs review over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Updated CR

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia
  • Pipeline finished with Success
    over 1 year ago
    Total: 508s
    #71928
  • Pipeline finished with Success
    over 1 year ago
    Total: 670s
    #71933
  • Pipeline finished with Canceled
    over 1 year ago
    Total: 48s
    #71969
  • Pipeline finished with Success
    over 1 year ago
    Total: 627s
    #71970
  • Pipeline finished with Success
    over 1 year ago
    Total: 627s
    #71977
  • Status changed to RTBC over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Thanks! CR reads well.

  • Pipeline finished with Success
    over 1 year ago
    Total: 731s
    #73270
  • Pipeline finished with Success
    over 1 year ago
    Total: 650s
    #75966
  • Status changed to Needs work over 1 year ago
  • 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.

  • Status changed to RTBC over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Rebase on 11.x

  • Pipeline finished with Success
    over 1 year ago
    Total: 852s
    #80486
  • Pipeline finished with Canceled
    over 1 year ago
    Total: 246s
    #85196
  • Pipeline finished with Failed
    over 1 year ago
    Total: 563s
    #85205
  • Pipeline finished with Success
    over 1 year ago
    Total: 562s
    #85229
  • Status changed to Needs work over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia dpi Perth, Australia

    Posted a lengthy review.

  • Pipeline finished with Success
    over 1 year ago
    Total: 481s
    #98211
  • Status changed to Needs review over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia
  • Pipeline finished with Failed
    over 1 year ago
    Total: 507s
    #98363
  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave
        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
    
  • Pipeline finished with Failed
    over 1 year ago
    Total: 571s
    #99835
  • πŸ‡¦πŸ‡ΊAustralia dpi Perth, Australia

    Added further feedback, tests still failing.

  • Pipeline finished with Canceled
    over 1 year ago
    Total: 286s
    #116045
  • Pipeline finished with Failed
    over 1 year ago
    Total: 489s
    #116049
  • Pipeline finished with Failed
    about 1 year ago
    Total: 195s
    #141208
  • πŸ‡ΊπŸ‡ΈUnited States dww

    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.

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    kim.pepper β†’ changed the visibility of the branch 11.x to hidden.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 200s
    #498824
  • Pipeline finished with Failed
    about 2 months ago
    Total: 131s
    #498834
  • Pipeline finished with Failed
    about 2 months ago
    Total: 234s
    #498838
  • Pipeline finished with Failed
    about 1 month ago
    Total: 173s
    #499995
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    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

  • Pipeline finished with Failed
    about 1 month ago
    Total: 582s
    #500018
  • Pipeline finished with Success
    about 1 month ago
    Total: 590s
    #500028
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

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

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 192s
    #502483
  • Pipeline finished with Failed
    about 1 month ago
    Total: 224s
    #502484
  • Pipeline finished with Success
    about 1 month ago
    Total: 654s
    #502493
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Thanks for the reviews. Resolved all threads.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan
  • Pipeline finished with Success
    about 1 month ago
    Total: 556s
    #502502
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    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?

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

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

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    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

  • Pipeline finished with Success
    about 1 month ago
    Total: 591s
    #502525
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    @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.

  • Pipeline finished with Success
    about 1 month ago
    Total: 809s
    #502573
  • πŸ‡ΊπŸ‡ΈUnited States dww

    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...

  • πŸ‡ΊπŸ‡ΈUnited States dww

    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. πŸŽ‰

  • πŸ‡ΊπŸ‡ΈUnited States dww

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

  • Pipeline finished with Failed
    about 1 month ago
    Total: 153s
    #503263
  • Pipeline finished with Failed
    about 1 month ago
    Total: 153s
    #503264
  • Pipeline finished with Failed
    about 1 month ago
    Total: 290s
    #503267
  • Pipeline finished with Failed
    about 1 month ago
    Total: 543s
    #503272
  • Pipeline finished with Success
    about 1 month ago
    Total: 464s
    #503281
  • πŸ‡ΊπŸ‡ΈUnited States dww

    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

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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!

  • πŸ‡ΊπŸ‡ΈUnited States dww
    • larowlan β†’ committed 76dcf5a2 on 11.2.x
      Issue #3410938 by kim.pepper, dww, nicxvan, smustgrave, andypost, dpi,...
    • larowlan β†’ committed 63740f3d on 11.x
      Issue #3410938 by kim.pepper, dww, nicxvan, smustgrave, andypost, dpi,...
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Committed to 11.x and backported to 11.2.x

    Published change record

    Thanks all!

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

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¨πŸ‡­Switzerland berdir Switzerland

    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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • πŸ‡³πŸ‡ΏNew Zealand quietone
  • πŸ‡ΊπŸ‡ΈUnited States xjm
  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    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.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

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

    Glad we identified the root cause.

  • πŸ‡ΊπŸ‡ΈUnited States xjm

    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. 🀣

  • πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

    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.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Thanks @jurgenhaas for the extra context

  • πŸ‡³πŸ‡ΏNew Zealand quietone
Production build 0.71.5 2024