Problem/Motivation

There is a new api for install time requirements, we should use this.

Steps to reproduce

  • layout_discovery
  • media
  • package_manager
  • pgsql
  • workspaces
  • testing_requirements

Functional updates:

  • Media had an unnecessary !empty($error), that was removed and baseline updated.
  • Package manager install check technically ran in all phases, but only really needs to run for install time.
  • Package manager similarly the FailureMarker check ran during install, but doesn't really make sense there.

I did not convert the following to leave tests for procedural:

  • requirements1_test

Created a follow up for: experimental_module_requirements_test #3513879: Convert experimental_module_requirements_test_requirements to new Class

Proposed resolution

Convert to the new class structure.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#22 3513410-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3513410

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !11504

nicxvan’s picture

nicxvan commented

I am still working on this.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented

I am still working through this.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented

Not sure I'm following the need to break up into 2 file locations now?

nicxvan’s picture

nicxvan commented

When it's for phase runtime or update they are hooks still so those go in the Hook namespace.
If they are install time then that is not a hook, it is a special class, the change record is here: https://www.drupal.org/node/3492429

smustgrave’s picture

smustgrave commented

Gotcha

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Mostly looks good, thanks! A few nits.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Applied almost all of them, I probably would have left most of those to follow ups since copy and paste issues like this are already hard enough to review.

I left one I didn't think was correct.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

Sorry, my 😉 about adding StringTranslationTrait was shorthand for "add this, then use it". But you only added, so phpcs is complaining about unused use. Can either revert that suggestion, or actually use the trait...

Thanks,
-Derek

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

Ah sorry, for some reason I was looking at the wrong class you removed the extra return, I applied that locally.

I also removed the string translation, I saw the other translation change and thought it was the complete suggestion.

I decided to remove it since this is part of the install time, and it's better to handle those updates in a follow up I think. For the bits that are outside of install time I think it's ok to add injection here.

dww’s picture

dww
we/he/they
commented
Title: Convert hook_requirements that do interact with install time that are not system » Convert hook_requirements() that do interact with install time that are not system
Status: Needs review » Needs work

Cool, thanks!

New day, fresh eyes, new MR threads. 😂

nicxvan’s picture

nicxvan commented
Related issues: +#3520888: Review duplicated requirements check in package manager, +#3520889: [pp-1] Check pgsl requirements check comparison, +#3520890: Reduce pgsql requirements duplication

Created the follow ups for the ones I think are out of scope.
Addressed other comments.

Ready for review again!

I'll keep an eye on tests.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review

Thanks for those follow-ups. Resolved all the open threads with links. Re-reviewing now.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

With all the follow-ups open, all my concerns have been addressed. I see no way to further improve this MR that's in scope. The pipelines are all green. RTBC!

Thanks,
-Derek

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3513410-nr-bot.txt91 bytes

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.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Appears to be random nightwatch failure, re-running is fine

Restoring previous RTBC status.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

One comment about duplication

dww’s picture

dww
we/he/they
commented
Status: Needs work » Reviewed & tested by the community

Replied re duplication. Back to RTBC.

dww’s picture

dww
we/he/they
commented

Sorry, wrong link. See related issues.
#3520890: Reduce pgsql requirements duplication

  • larowlan committed 578f6f36 on 11.x 
    Issue #3513410 by nicxvan, dww, larowlan: Convert hook_requirements()...

larowlan closed merge request !11504

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and unpostponed the follow up

Thanks

dww’s picture

dww
we/he/they
commented
Parent issue: » #3490845: [meta] Convert hook_requirements to new class or hook_update_requirements or hook_runtime_requirements

Thanks!

Giving this its proper parent issue for posterity...

Status: Fixed » Closed (fixed)

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