Problem/Motivation

Many hook_requirements implementations have dependencies that are not explicit, and are difficult to test due to a lack of dependency injection.

These issues provided OOP replacements for the procedural hook_requirements():

This plan is for converting all of core to use these OOP replacements.

Steps to reproduce

N/A

Proposed resolution

See child issues for each part of the plan.

Remaining tasks

Already converted

  • file

Runtime phase only
#3500632: Convert hook_requirements() that do not interact with install time

  • image
  • jsonapi
  • locale
  • mysql
  • navigation
  • node
  • search
  • update_test_schema
  • update
  • user
  • demo_umami

Update phase only
#3500632: Convert hook_requirements() that do not interact with install time

  • update_script_test

Includes install phase
#3513410: Convert hook_requirements() that do interact with install time that are not system

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

Experimental Module Requirements Test

mysqli

System

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Title: [pp-3] Convert hook_requirements to new classes or hook_runtime_requirements » [pp-3] Convert hook_requirements to new class or hook_update_requirements or hook_runtime_requirements
Issue summary: View changes
dww’s picture

dww
we/he/they
commented

For scope, wondering if we really need separate issues for all of these. E.g. the one for update (status) will be a simple move to an OOP hook_runtime_requirements(). Maybe we should group by all modules that only implement a single phase in their hook_requirements()?

  • Only needs hook_runtime_requirements()
  • Only needs hook_update_requirements()
  • Only needs the hook_install_requirements() class
  • Separate issues for each module that implements more than 1 phase

I can't wait to see system_requirements() refactored. That's going to help so much. I might even be inspired to raise my hand to at least start on that one. ;)

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Opened #3493718: Convert system_requirements() into OOP hooks and install time class to track it. Adding to summary.

nicxvan’s picture

nicxvan commented

Yeah that split was my real plan, glad to see some momentum here!

nicxvan’s picture

nicxvan commented

I think this is no longer postponed, but we should only create three children for now.

1. Convert all hook_requirements that only interact with phase update
2. Convert all hook_requirements that only interact with phase runtime
3. Convert hook_requirements that interact with both runtime and update if any.

Discuss here any that interact with install in any way. Discuss whether we convert those phases or wait on #3490843: Create class to replace hook_requirements('install')

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented

There is literally one implementation in core that only interacts with update, we should just convert all requirements unless they interact with install.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Title: [pp-3] Convert hook_requirements to new class or hook_update_requirements or hook_runtime_requirements » [meta] Convert hook_requirements to new class or hook_update_requirements or hook_runtime_requirements
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Related issues: +#3490843: Create class to replace hook_requirements('install'), +#3490846: Deprecate hook_requirements
nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Related issues: +#3513410: Convert hook_requirements() that do interact with install time that are not system, +#3513410: Convert hook_requirements() that do interact with install time that are not system, +#3493718: Convert system_requirements() into OOP hooks and install time class
dww’s picture

dww
we/he/they
commented
Related issues:

Removing duplicate related issue

nicxvan’s picture

nicxvan commented
Issue summary: View changes
Related issues: +#3513879: Convert experimental_module_requirements_test_requirements to new Class
nicxvan’s picture

nicxvan commented
Related issues: +#3500632: Convert hook_requirements() that do not interact with install time
dww’s picture

dww
we/he/they
commented

These are now committed to 11.x:
#3500632: Convert hook_requirements() that do not interact with install time
#3513410: Convert hook_requirements() that do interact with install time that are not system

That leaves these as the only blockers to #3490846: Deprecate hook_requirements, right?

#3513879: Convert experimental_module_requirements_test_requirements to new Class (trivial and RTBC)
#3493718: Convert system_requirements() into OOP hooks and install time class (massive and NW)

Wonder if it's possible to finish #3493718 in time for 11.2.0-beta, which would allow us to deprecate hook_requirements() still in 11.2.0 for removal in 12. Otherwise, I think that'd be a disruptive enough change that it'd be a 11.3.x deprecation for removal in 13...

Thanks,
-Derek

p.s. Also wonder why we're marking child issues as related. 😉

dww’s picture

dww
we/he/they
commented
Category: Task » Plan
Issue summary: View changes

Minor edits to the summary to reflect reality, and converting this meta into a plan.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented

Also wonder why we're marking child issues as related.

I've never been super clear on the difference so I usually default to related.

As for system_requirements, I'd love to get that in, but it's massive, and I feel like the template preprocess conversions will be easier to get in.

dww’s picture

dww
we/he/they
commented
Related issues: -#3513410: Convert hook_requirements() that do interact with install time that are not system, -#3493718: Convert system_requirements() into OOP hooks and install time class, -#3513879: Convert experimental_module_requirements_test_requirements to new Class, -#3500632: Convert hook_requirements() that do not interact with install time

Is there a meta or plan for template preprocess conversions? I haven't been following those. Happy to try to help if I can.

Re: child vs. related, there's no hard/fast rule. But generally, if something is a "follow-up" that'd be a child issue. Especially for meta/plan issues, all the steps in the plan should be child issues. Related is for exactly that: related. It's not directly a "descendent" or "ancestor" of the issue, but it's somehow relevant to also consider for whatever reason(s).

For this plan, removing all the related issues that are already children so they're not listed twice. The remaining two are "siblings" of this issue (other children of the same parent). 🤓 But for better visibility, might as well leave those. 😉

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Active » Fixed

Well it turns out system_requirements got in first!

Here is the meta you asked about: #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent

All hook_requirements in core are converted, I'm going to wrap this!

Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented

Go team!

Status: Fixed » Closed (fixed)

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