- Issue created by @kim.pepper
- Merge request !5958Issue #3410938 Create enums for RequirementSeverity and deprecate constants β (Closed) created by kim.pepper
- Status changed to Needs review
over 1 year ago 10:52pm 26 December 2023 - π¦πΊ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.
- π¦πΊ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 7:09pm 27 December 2023 - πΊπΈ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.
- Status changed to Needs review
over 1 year ago 3:00am 3 January 2024 - π¦πΊ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 2:48pm 3 January 2024 - πΊπΈUnited States smustgrave
BC tests added look great! Moving to NW for the CR.
- Status changed to Needs review
over 1 year ago 11:16pm 4 January 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Updated CR
- Status changed to RTBC
over 1 year ago 3:02pm 5 January 2024 - Status changed to Needs work
over 1 year ago 5:50pm 20 January 2024 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 10:22pm 21 January 2024 - π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Rebase on 11.x
- Status changed to Needs work
over 1 year ago 1:35pm 13 February 2024 - Status changed to Needs review
over 1 year ago 7:18am 19 February 2024 - Status changed to Needs work
over 1 year ago 3:58pm 19 February 2024 - πΊπΈ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
- π¦πΊAustralia dpi Perth, Australia
Added further feedback, tests still failing.
- πΊπΈ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.
- π¦πΊ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 - π¦πΊ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.
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
Thanks for the reviews. Resolved all threads.
- πΊπΈ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
- π³πΏ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.
- πΊπΈ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 β - πΊπΈ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
Fleshing out API changes section of summary.
-
larowlan β
committed 76dcf5a2 on 11.2.x
Issue #3410938 by kim.pepper, dww, nicxvan, smustgrave, andypost, dpi,...
-
larowlan β
committed 76dcf5a2 on 11.2.x
-
larowlan β
committed 63740f3d on 11.x
Issue #3410938 by kim.pepper, dww, nicxvan, smustgrave, andypost, dpi,...
-
larowlan β
committed 63740f3d on 11.x
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Committed to 11.x and backported to 11.2.x
Published change record
Thanks all!
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
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.
- π¦πΊ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 fordrush 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