Replace hook_requirements with tagged services

Maybe it could use attribute based discovery instead?

Hmm. Having a look at 🌱 [META] Hooks via attributes on service methods (hux style) Active this way more complex that I imagined.

@larowlan pointed out in slack that 'install' checks for hook_requirements run _before_ a module is enabled/loaded, so we can't use services for this.

Yes, there's no services before installing but composer namespaces are setup-up so discovery is possible

Related 📌 Deprecate drupal_check_module() and replace with a service Needs review

Hmm. I guess we could something like:

$requirements = $classResolver->getInstanceFromDefinition("\Drupal\{$module}\Requirements");

That way classes implementing ContainerInjectionInterface could have other non-module services injected.

I wonder how attributes would help?

Not sure whether this is worth pursuing or not. 🤔

Would you mind my adding this as part of Follow up 3? 📌 [PP-1] Explore converting Install hooks to OOP Active

There are actually two parts to hook requirements, there is a hook requirements for runtime that can be OOP and hook_requirements during install which likely cannot be converted.

Make that three parts:
Install
Update
Runtime

The first two likely need to be specially named classes we can load.

The last can be a hook attribute no problem.

Needs review for process

@kim.pepper would you mind moving your experiment over to 📌 Create class to replace hook_install_requirements Active

I've handled the update and runtime ones. If we get the helper class then we can work on converting and deprecating hook_requirements.

We'll need to decide if we care about BC toward invocations of the requirement hook. It probably isn't used that often, but there are for example a bunch of monitoring related modules that are collecting requirements as well, for example: https://git.drupalcode.org/project/monitoring/-/blob/8.x-1.x/src/Plugin/....

That is going to break as modules are converted away from hook_requirements(). We can say that's OK, but it's something that needs a decision.

Second thought, if we're going to change this anyway. A few hook implementations, like system_requirements(), are huge, do many different things. That also makes them harder to test. If we refactor it, should we consider a more atomic API where a single method is responsible for a single "thing"?

It's possible, however I suspect this will only support one implementation even in OOP just like hook_mail and the other couple of implementations.

system_requirements will require thought, but I think even refactoring to move the pieces that bridge update and install or runtime to an external call will make it much more readable.

As for BC we need to consider this for procedural hook deprecation in general since once we deprecate them, any module doing something like this: $function = $module . '_requirements'; will break unless it's for the specified install hooks.

I was actually thinking about writing a comment on something like this on 📌 [Plan] Determine how to deprecate procedural hooks. Active when I saw this.

I think this will just be a BC break. We also have to consider \Drupal::moduleHandler()->invoke

@berdir,to answer your question more fully, I think it has to be a BC break here, I don't see a way around it unless I'm missing something. But what can happen is you can update that module to handle it.

You should be able to update that module with the following:

\Drupal::moduleHandler()->invoke($module, 'requirements', ['runtime'])
Store that results
Then call: \Drupal::moduleHandler()->invoke($module, 'runtime_requirements')

Merge the requirements and do what you need to.
Just be aware that right now LegacyInvoke fallback returns NULL if no hooks are implemented, so you'd have to handle that.

Ok the three phases are all ready for review.

The install time class needs review for approach.

I think it would be useful to convert system_requirements(), the by far most complex implementation of this hook to understand the implications of this.

It's a massive function (1500 lines) and one thing is that there is a large overlap between then different phases and code is often shared. e.g. the PHP version is always reported for runtime, but all then complain if it doesn't match. update and runtime can share code fairly easy, but install is separate.

That said, system_requirements() is also an extreme edge case and all other implementations are far simpler. So maybe we should just split that differently anyway, move it into a component or something. It's really not module requirements, they are the requirements of Drupal as a whole. We could also consider multiple hook implementations to split it up, but that will then break the ability to use it with invoke() completely. There are also 20 calls to Drupal::service() in there, although not all of them are to different services.

IMHO, if we just split those 1500 lines into 3x 800 lines or something, then we won't really gain that much and keeping those things in sync will be more work in the future.

Yeah system requirements will be it's own issue, I'm not really looking forward to that.

That will be part of Phase 2 though, it has to be.

My plan is to look through the hook and split it based on phase, for the sections that have more than one phase that will become a helper called by both.

Assuming it's part of update or runtime we can inject those service calls. If it's part of the install time we'll need to review it.

The truth is the 1500 line function isn't super readable now, it should be much easier when it's structured in the new hooks.

That will take significant effort.

I have a scope question at #3490845-4: [pp-3] Convert hook_requirements to new class or hook_update_requirements or hook_runtime_requirements → on how to best split that up. However, definitely system_requirements() will be a huge effort, so I already opened 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active for it and added to #3490845. I'm actually kind of excited about that one. 😅 Maybe I'll be inspired to find time to take an initial stab at it.

> Yeah system requirements will be it's own issue, I'm not really looking forward to that.

Certainly a separate issue. But I also think we need to have a plan for it now, to be able to verify the proposed architecture. As mentioned, a possible answer can be that system_requirements() does not fit into that architecture and we move at least parts of it out into the installer or so. Again, system is also special in that it's not just a huge amount of code, but it's also called super super early in the installer, not sure how that works exactly, but I wouldn't be surprised if we run into issues there calling it as a class like that.

I'm torn between a minimal change to move this as quickly as possible to the new system, as it's the only remaining regular hook not supported. But this also seems like a chance to actually improve the API around it. The array keys and constants used here are hard to remember, I always have to look up stuff. Converting that into a class would be a big DX improvement for example. I suppose we could still introduce such a class later, but it's also going to break more stuff (which might be OK, since directly invoking a hook isn't really an API that we provide BC for)

@berdir: I warmly welcome you (or anyone else) to start sketching out a plan at 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active . I doubt we actually want to open an MR there for a bit (hence [pp-3]) but we could already start fleshing out the summary now to do what you're talking about in #32. I agree that'd be helpful work to verify the API being proposed here makes sense, which is why I already opened it, even with my concern about opening sub-issues of 📌 [pp-3] Convert hook_requirements to new classes or hook_runtime_requirements Active for every core module.

I'm torn between a minimal change to move this as quickly as possible to the new system, as it's the only remaining regular hook not supported. But this also seems like a chance to actually improve the API around it.

I'm a fan of incremental improvement.

I think this is out of scope for this issue, but for the future we can add helper methods for the install time or subclass Hook to add those same helpers to update and runtime.

This is already going to be a complicated conversion, let's not make it harder with api changes, this already makes the experience better.

@berdir: I warmly welcome you (or anyone else) to start sketching out a plan at #3493718: [pp-3] Split up and refactor system_requirements() into OOP hooks.

I decided to skip the sketching and plans for now and just try it, with the install class as a starting point. What I'm looking for is to get a feel of how that code will look like, how much code duplication there is and so on. It didn't seem that a sketch/plan could really give me an impression of that. system_requirements() is a monster, but 80-90% of it is large chunks of checks that apply to 1, 2 or 3 of the phases.

It took me a few minutes to copy & paste system_requirements() to the Install\SystemRequirements class, go through those chunks and delete all that didn't apply, partially thanks to how PHPStorm code collapsing works, I can just close an if () and delete it without having to match curly brackets manually. The result is ~500 lines of code in that class. I expect this is a throwaway branch. I took some shortcuts (stale comments and so on). There are a few parts that are trickier because it's mostly the same, but some descriptions are different, or it's an error at runtime and a warning on install. This is where we'll end up with code duplication.

Opened a MR in the system issue, might also do runtime/update later but it's getting late now.

I'm a fan of incremental improvement.

Same, and I've been frustrated in the past when what I saw as incremental improvements wasn't good enough and issues got stuck.

The other side of the coin to incremental improvements is BC. The changes that we'll make further improvements to the requirements stuff is slim, we'll need additional BC layers and have multiple deprecations in overlapping minor versions. Might be a bit of a now or never.

I have some more thoughts on whether or not we should split both the hooks and the work to implement them, but I need more time to properly write that down.

Also tried runtime and update now. Especially runtime was more work but the duplication between the two methods isn't as bad as I thought. There was one 300loc chunk that I split into a shared method. An option would also be to define that as a separate hook that both implement, but that will break using ->invoke() at the moment. I do wonder if we should change invoke() to support multiple hooks and merge the results together, a bit like invokeAll()? Would work well for requirements. and then we could split the still huge method up into logical chunks simply be having multiple hook implementations.

There are several chunks between 20-100 lines of code that is shared between the two methods.

I expect reviewing the conversion will be more challenging than actually making the changes. A lot of the stuff going on in those requirements checks isn't tested because it's about it's about infrastructure and things that we can't really simulate at the moment.

One concern about this approach is dependency injection in combination with update requirements. Not as severe as install-time requirements, but update also runs in a reduced and possibly fragile environment. If we inject 10 classes there, then possibly a future update could break that. We could put them in separate classes but then the sharing part won't really work that well anymore.

I was wondering if it really makes sense to split runtime and update, the minimal fix for the problem here would be to keep $phase and just move install out. But I expect system is going to be pretty much the only case with more complex variations between the phases, pretty much all other core and contrib implementations will just either run or not on specific phases and if it's both runtime and update it's easy to register both for a single method. One downside is that rector support will be much more complex, it will not be able to untangle conditional early returns or switch statements or stuff like that. We could fake it by keeping an existing function with $phase and register two calls to it with the respective phase. Also legacy support is tricky for this. But I also suspect we won't support that at all in rector, there aren't that many requirements hooks compared to others.

One thing I noticed is that merging of the return values is inconsistent, install hooks seem to be a regular array_merge(), while others use mergeDeep(), but I'm not sure we really want to do that, you should not attempt to merge specific definitions? use the alter hook for that.

Thank you for doing this, I think this validates the approach and your points warrant resolution.

An option would also be to define that as a separate hook that both implement, but that will break using ->invoke() at the moment. I do wonder if we should change invoke() to support multiple hooks and merge the results together, a bit like invokeAll()?

I don't think that is possible at the moment, I really think we just need to keep these hooks as single implementation. The disruption this would cause would be far larger than the gain in splitting system.

One concern about this approach is dependency injection in combination with update requirements. Not as severe as install-time requirements, but update also runs in a reduced and possibly fragile environment. If we inject 10 classes there, then possibly a future update could break that. We could put them in separate classes but then the sharing part won't really work that well anymore.

This is the main reason I split update out into a separate one, so those restrictions can be defined as necessary. If we think the risk is high enough we can move it to a class, but that would be weird because modulehandler already invokes update. I think it should remain as is, but document it better maybe?

I was wondering if it really makes sense to split runtime and update, the minimal fix for the problem here would be to keep $phase and just move install out.

I think almost all modules implement only one phase, and they don't consider the bits you mentioned, I think splitting it is the less disruptive option.

One thing I noticed is that merging of the return values is inconsistent, install hooks seem to be a regular array_merge(), while others use mergeDeep(), but I'm not sure we really want to do that, you should not attempt to merge specific definitions? use the alter hook for that.

That wasn't intentional, are you recommending just use array_merge for all three scenarios?

I'm going to update those branches to use array_merge

I found this issue via the issues the create hook_*_requirements(). I'm not sure about the focus of the API design here which feels mostly centred on how can we possibly convert hook_requirements to OOP and not around what's the best API for gathering requirements from a module. This issue has some of the problems with moving to OOP - i.e. dependency injection in a fragile and changing environment being fraught and that's good to see. And I agree that anything that makes system_requirements easier to manage would be great but I'm not sure the answer is split it up into 3 separate things and have massive duplication.

I think we need to make it possible for the same requirements code to be run on install, update and runtime phases so, for example, we don't have three different hook implementations that do the PHP extension checking that's done currently in system_requirements().

Wrt to dependencies the following code makes me fear for what we're going to have deal with:

  // Test if configuration files and directory are writable.
  if ($phase == 'runtime') {
    $conf_errors = [];
    // Find the site path. Kernel service is not always available at this point,
    // but is preferred, when available.
    if (\Drupal::hasService('kernel')) {
      $site_path = \Drupal::getContainer()->getParameter('site.path');
    }
    else {
      $site_path = DrupalKernel::findSitePath(Request::createFromGlobals());
    }

The idea of the phase being runtime and there's not always a kernel service is mind boggling.

First let me start with the main driver of this.

If we ever want to deprecate procedural hooks we need to do something about hook_requirements.
I initially split it like this because modulehandler already invokes requirements('update') and requirements('runtime').

Then I split install into a special class because it was called directly as a magic function in the install processes.
Next I reviewed core requirements and noticed almost all interact with one phase, system requirements is a beast, but berdir did a naive conversion to this structure and it passes: 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active

Now to address the comment here:

I think we need to make it possible for the same requirements code to be run on install, update and runtime phases so, for example, we don't have three different hook implementations that do the PHP extension checking that's done currently in system_requirements().

I don't think that's possible, this gets called during profile dependency checking as well, this is another reason I split them, they have been combined, but they do have different purposes.

If they are separate we can begin to determine what things are safe to inject for each phase.

The idea of the phase being runtime and there's not always a kernel service is mind boggling.

That can't be right, but I bet it's due to being mixed in with system requirements.
I didn't see why that was added, but it was done as part of this #2384675: Deprecate conf_path() →

I think hook_requirements should be split just to remove the questions of what is safe to inject per phase and to remove confusion on what is happening in a given hook_requirements.

Personally I'd postpone the rearchitecture to followups once we have the scope of what is available to each phase.

If there are existing issues discussing long term updates I'd love to read them and see if there is a way to incorporate those here.

I disagree with the assessment that they are separate things. Sometimes they are and sometimes they are not. That's why I gave the PHP extension checking example in my comment. That is checked regardless of phase.

If we look at a contrib example:

/**
 * Implements hook_requirements().
 */
function commerce_requirements($phase) {
  $requirements = [];
  if ($phase == 'install' || $phase == 'runtime') {
    if (!extension_loaded('bcmath')) {
      $requirements['commerce_bcmath'] = [
        'title' => t('BC Math'),
        'description' => t('Commerce requires the BC Math PHP extension.'),
        'severity' => REQUIREMENT_ERROR,
      ];
    }
  }

  return $requirements;
}

This shows how things are flawed currently - the BC Math extension is just as much required during the update phase as it is in the install and runtime phase - imagine an update function doing something with data relying on the BC math depending code for example.

My thinking in these cases is that it would move to a helper.

If it's in the update or requirements then it can just be a helper function in the hook class.

If install is one of the shared phases then it can be a static helper function.

In your example I would create a static helper method that would call this:

  if (!extension_loaded('bcmath')) {
      $requirements['commerce_bcmath'] = [
        'title' => t('BC Math'),
        'description' => t('Commerce requires the BC Math PHP extension.'),
        'severity' => REQUIREMENT_ERROR,
      ];
    }

Then just call that static helper in each one.

Another option is to move them all to a requirements class and remove the whole hook_requirements, then we have a method for install, update, and runtime, then any shared functionality like this is an internal helper.

We likely wouldn't be able to inject anything, but that is no worse than what we have now.

I had concerns at 📌 Create hook_runtime_requirements Active but it seems more appropriate to raise them here. As usual, I find @alexpott's views compelling, and perhaps a more clear articulation of what I was worried about.

Generally, I didn't love seeing class FileRuntimeRequirements... seems we want to move to class FileRequirements, and have separate methods for each phase. We should be making it easier and cleaner to share code between the phases. Probably more requirements checks should be happening at multiple phases than is currently the case.

I'm not sure how common it is to need services in these checks.

Can we say that if your Requirements class needs services for update and runtime, which it can't use during install, that you have to have a service getter/setter pair, and that getContainer() returns something valid for during those phases and NULL during install? Tests can call the setters with mocked services. The real code will use getContainer() or whatever.

But yeah, I think it'd be better DX overall to move from a single hook_requirements($phase) to a single new requirements API, not 2 new hooks and a 3rd thing implemented some completely other way.

If install requirements are such a special case, and it really does make sense to handle it separately, at least I think we should unify the hook-based parts to encourage more sharing.

Thank you for sharing your thoughts. I think there are a few approaches we can take and we just need to choose one and work on it.

Before I lay them out, just to clarify FileRuntimeRequirements could have been named anything, it's a hook class so the class name is arbitrary, it's the #[Hook('runtime_requirements')] that makes it work in the current iteration.

Some of these points are duplicated because they are relevant to more than one approach.

Approach 1

Special class for Install time requirements, new separate hooks for runtime and update requirements.
Pros:

• It's already done
• It's straightforward to convert from current hook_requirements to this system even in complex places like system_requirements as evidenced by @berdir's exploration there
• Install is no longer a hook so you get separation to call out it's unique
• We can document limitations of each phase better than we have in the past
• We can eliminate hook_requirements

Cons:

• It is a little confusing that one is not a hook and two are
• It is harder to share code between phases (especially if one of those phases is Install)

Approach 2

Special class for Install time requirements, new hook that combines runtime and update.
Pros:

• It is easy to convert the current MRs
• It keeps update and runtime closer to what they are today
• It's straightforward to convert since any logic you have for both runtime and update can be copy pasted
• We can document limitations of each phase better than we have in the past
• We can eliminate hook_requirements

Cons:

• It is a little confusing that one is not a hook and two are a new hook
• It is harder to share code between install and the hook
• I think it is confusing to have multiple phases in one hook

Approach 3

Special class that has a different method for each phase. Documented way to do injection that is safe for install time.
Pros:

• All requirements are in a consistent place
• We can document limitations of each phase better than we have in the past
• We can share code between phases easily (unless we share something using services with install)
• We can eliminate hook_requirements

Cons:

• It needs work
• It will be far more complex to set up dependency injection for update and runtime because you always need to be safe for install even if you don't use it
• Integration where update and runtime are called is far more complex
• We can share code between phases easily, this means it's more easy to add a service that isn't available install time

Approach 4

Something else

Additional thoughts

A few other points, in most core modules I noticed that modules use one phase.
That means any of these approaches will work equally well for those.

The more I think about this the more I think that these really should have never been combined. The things you can do with requirements is very different in install than the other two.

I still think Approach 1 is the clear winner with approach 2 following close behind.
I think Approach 3 is only viable if we're using it as a bridge to completely overhaul the requirements api. I think the complexity it forces on runtime and update along with the complexity it adds to where we invoke them makes it not worth it here.

If I'm missing something let me know, while I do think approach 1 is the way to go, my main goal is to deprecate hook_requirements and I don't use hook_requirements that often myself so if others think one of the other approaches is better we can work towards that.

I am drawn to the benefits of handling install separately. You make a good case that it’s unfortunate to complicate DI for the runtime / update checks when in many cases those don’t care about install checks.

So even though it’s two independent systems, it might be better DX overall to have the simplicity of the hook for runtime and update cases…

I understand about the class name with the hook approach. I’m okay with putting the right Hook attribute on separate methods for each phase in the same ModuleRequirements class, even if that’s in the Hook namespace and not directly shared with the Install requirements.

I don’t mind 2 hooks (approach 1 over 2). Makes it easy to do whatever you need, and easy to share between them if we default to ModuleRequirements and methods per phase. I don’t think there’s much benefit in approach 2. Seems 1 or 3 would be preferable to 2.

I reserve the right to change my mind if Alex (or anyone) comes up with a better option 4. 😂

Thanks,
-Derek

Just reviving this a bit: I had asked @alexpott for his thoughts so copying his comment here since I think it's relevant and @dww already cross posted.

My general thought is that I think designing an API for requirements should not be driven by the container.

I'm not quite sure what this means. Any clarification would be great.

And I don’t think the idea of the static method doing the update check for all three phases is great.

I think this means approach 3 is out.

Another thing that needs to be addressed is hook_requirements_alter()

The approach I took does address this.

the massive problem with requirements is not really the container but the fact that we need to run code from a module before the module is installed.

It's not just before it is installed, it can be as Drupal is being installed, that is why I took Approach 1. Any issues during Drupal or Module install time can be documented and addressed on the class. If hook_update_requirements has any issues then it can be addressed and documented, and same for hook_runtime_requirements, though I don't anticipate anything ever going wrong there.

I still think Approach 1 is the a good step towards clean up. There are not many modules that interact with more than one phase, and the complexity keeping it consolidated I don't think is worth the slight clean up modules like commerce would receive.

If the first point can be clarified then maybe I am missing something on a way to unify them and replace hook_requirements.

I've been reflecting on this issue quite a bit. I think the install time requirements check prior to a module being installed or while Drupal is being installed is incorrect. We should never allow code from a module to be run before the container is aware of the module and its services - that's a recipe for complexity and bugs. If we look at the commerce_requirements example - it should not exist. This requirement should be part of the module's composer.json and we should be done. This line of thinking means I now agree that install time requirements should be treated separately from update and runtime. Because once we've made it separate we can deprecate it in 11.x and then remove it completely.

I think update and runtime should be managed together. The use-case for them being separate is pretty moot and we already have code to handle missing services when the update container is built.

Great thanks! Sounds like a vote for approach 2.

I'll create a new issue combining runtime and update, called requirements_check.

I also realized that we can always create hook classes for each phase in a follow up which would improve DX for modules interacting with a single phase.

@nicxvan I think the phase thing is overblown - the commerce example above should not be checking phase at all. If the extension is missing and you have commerce installed you shouldn't be able to run updates without bcmath (and yes this should be a composer requirement and the code removed but that's a tangential issue). In most instances for contrib, any requirement for runtime is a requirement for update. Sure system_requirements() has a load of complexity around what is checked when but even there we get it wrong. There is no reason why database json support is not checked on install. Also most system_requirements() should go into /core - it has no business being part of the system module.

Oh I see, so just requirements_check, no phase, and invoke it during update and runtime,

Then install time gets the class which we deprecate shortly after.

That feels cleaner, let me know if I missed something.

This requirement should be part of the module's composer.json and we should be done.

I don't think this can work, because composer.json is project-level, not module level, and a sub-module could have different requirements.

But... we could parse and validate composer.json ourselves, or we could add some support to .info.yml, so that the information is in a static file and not via executed PHP. Probably needs a new issue to discuss. Agree it would be nice to get rid of the 'install' phase but don't think it's going to be easy.

@catch, but a module can add a php library to their json and I think that can be resolved during build, however for this case I think we can move forward with the modified approach 2, and discuss this bit during the deprecation issue for the install time class. If it turns out we need to keep the class for certain cases then we can keep it.

I'm working on this new approach here: 📌 Replace hook_requirements with tagged services Active I will update the IS once I got it running.

@catch generally I think we should say that sub modules don't get to have different PHP version / extension / composer dependency requirements. Because all those should only be defined in the composer.json in the future. That's why it is a sub module :) The one exception to that would be an optional integration with another module. So module dependencies still belong in the .info.yml but everything else needs to be in the root project composer.json. This is the case already if a sub-module has a dependency on a composer PHP library that the main module does not.

A common pattern for sub-modules is to define the composer dependencies as 'suggest' and add a requirements check for the existence in the sub-module.

Partially caught up on this.

> I've been reflecting on this issue quite a bit.

Same. I think I have similar opinions and thoughts as @alexpott, not entirely though.

One thing is I think that system requirements should not be used as an example on how we want to define the API for modules. It's not requirements for the system module, it's requirements for Drupal as a whole, and and as such unique. It definitely has all kinds of valid combinations of phase specific checks. An example for update specific things is checking update hook/function compatibility, like having a schema version recent enough to support update. That's not something we want to run every time you visit the status page. But we do not have to make this fit the API, we can move it out of system_requirements() and put it in an update-specific place that we call explicitly. It might make sense to do this before introducing the replacement for hook_requirements().

I agree that (pre) install time stuff is tricky and annoying to support, but right now I think it does serve a purpose. I don't really agree with #55:

> I think we should say that sub modules don't get to have different PHP version / extension / composer dependency requirements. Because all those should only be defined in the composer.json in the future. That's why it is a sub module :) The one exception to that would be an optional integration with another module.

That one exception is actually quite common, and it's not limited to modules. You can also have a submodule that requires a composer dependency to do something, or a php extension. I just added such a module to the monitoring project. You could argue that it should be a separate project, but I'm not really willing to split development into multiple projects. Per #58, a common approach for that is indeed using suggests, but there are a number of alternatives. I'm almost certain we have an issue that wants to add support for required extensions to .info.yml. We could add support for composer.json instead, there are also open issues about moving at least part of what's currently inside .info.yml into composer.json files. The interesting bit is that drupal.org already now exposes each submodule as a separate composer.json, I'm not sure if it supports merging in an existing composer.json. If not it probably could be implemented, and drupal core definitely could/should check a composer.json file for dependencies. So arguably drupal.org already kind of supports a concept similar to git subtree splitting, except just for composer, which is sufficient?

That would cover most cases why you currently need install requirements. There are likely still going to be special cases though: The redis project even requires one of two extensions OR a composer dependency. You won't be able to declare that in composer.json/info.yml. I don't think it bothers with checking that atm, because it needs settings.php configuration to actually do anything anyway.

If we want I kept the two issues for Approach 1 with the split update and requirements hooks. That is probably the lowest risk version of this.

It's also super easy with OOP hooks to unify the hooks for most modules, just add both attributes to the same method:

#[Hook('runtime_requirements')]
#[Hook('update_requirements')]
public function requirements() {}

That gives the most flexibility moving forward.

If we really do want to unify them like we did in Approach 2 and there are other modules doing something similar to system different in updates vs runtime, Approach 2 is either a breaking change or we need to introduce phase for runtime and update which I think is not a great option, if we set it up so that people need to swap. I agree with berdir that system shouldn't be the litmus test for this to be honest.
Here are the two approach issues if others want to take a look:

Approach 1
Install (same for both approaches) 📌 Create class to replace hook_install_requirements Active
Runtime 📌 Create hook_runtime_requirements Active
Update 📌 Create class to replace hook_update_requirements Active

Approach 2
Install (same for both approaches) 📌 Create class to replace hook_install_requirements Active
Combined runtime and update ✨ Create hook_requirements_check Active

I can create an issue to look at moving the update bit out of system_requirements, the problem I see is it currently happens in update.inc, would we want to preserve the ability to alter it if we move it to a service?
Approach 1 let's us shift it over as is with minimal disruption to core and contrib.

In my previous comment, I didn't really mean to point out a problem with removing the phase, more the opposite, by saying just because system_requirements() might need it, that might not be a reason to keep it.

However, I realized something that I believe is a hard blocker for removing the phase argument/separate hooks. Sure, the example in #42 wouldn't need to consider phase at all. However, the difference between update and runtime is that errors during phase update are *blocking* you from updating. We have many runtime errors that must _not_ block you from updating. Take a look at update_requirements() for example. Having (additional) available security updates must _not_ block you from running updates for the modules you already did update.

I don't see how that can possibly go away.

So yes, I think we either need to keep the argument or we need two hooks for update and runtime. Again, system_requirements() is a weird special case and we should split that up, but the new hook system already gives us a number of options to dealing with that, per #60:

1. You can implement both hooks with a single method, that is sufficient for most simple requirements implementations.
2. You can implement multiple hooks with varying combinations of one or both hook registrations.
3. You can even do the following, that might be easiest way for system_requirements() as a first step (and we can look into splitting up later) and it might also be an option for rector, with a comment to suggest to simplify it as feasible:

class SystemRequirementsHooks {

#[Hook('update_requirements')]
function updateRequirements() {
  return $this->requirements('update');
}

#[Hook('runtime_requirements')]
function runtimeRequirements() {
  return $this->requirements('runtime');
}

function requirements($phase) {
  ....
  // 1:1 copy of existing system_requirements() minus install-only parts.
}}
}

> 1. You can implement both hooks with a single method, that is sufficient for most simple requirements implementations.

FWIW, if we except this to be the default then we could also keep a single hook with an argument. We could possibly even keep the name, we'd just stop calling the new hook with phase install.

But I think the blocking thing with update is worth to separate to not make mistakes around that. Especially if improve default support for explicitly checking extensions/composer requirements, update should probably focus on things that are _actually_ relevant when updating, to not overload you with irrelevant warnings and information. So I think that's a good argument to have two separate hooks and say only implement update_requirements if it's actually relevant to the update process.

layout_discovery_requirements() is also an interesting case for hook_install(), it's a conflict-check and not a dependency check. Preventing install if another module is around. Same for workspaces. So if we want to support that we'd need another new .info.yml thing.

Searching for \$phase ===? 'update' finds only a single case that explicitly checks for update outside of system module, and that's valid, google_tag that checks things around google_analytics.

For me that confirms that two separate hooks (and a separate solution for install) are likely the most sensible way forward. runtime requirements should cover 80-90% of the cases outside of system_requirements() and probably 90% if we support install time extension/composer dependency checks. And for system we can for example start with option 3 above.

However, the difference between update and runtime is that errors during phase update are *blocking* you from updating. We have many runtime errors that must _not_ block you from updating.

Yes this is important to keep.

layout_discovery_requirements() is also an interesting case for hook_install(), it's a conflict-check and not a dependency check. Preventing install if another module is around. Same for workspaces. So if we want to support that we'd need another new .info.yml thing.

Adding conflicts: to .info.yml or something like that seems good to me.

I postponed the combined approach.

I like the way you organized the system conversion in 61.

I think keeping a phase argument would be a mistake, it forces most implementations to immediately have a guard clause.

Is there enough agreement for me to unpostpone approach 1 and update the IS? It feels definitive to me.

Updating the IS for Approach 1 again.

> would we want to preserve the ability to alter it if we move it to a service?

This was specific to the idea of moving system requirements stuff out of the hook, but I think it's a question that's worth asking in general.

Use cases for altering requirements seem rather rare. I can maybe see some for runtime hooks, but update?

Looking at https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22Imple..., there are exactly four in contrib, although possibly quite a bit more in custom code where people use it to remove undesired things?

Maybe we could skip adding the alter for update. On the other hand, it costs us little to have the hook and if people do weird stuff with it then that's on them?

Yeah it's weird, we can remove it, but it is technically supported now and as you say it's kind of caveat emptor.

Looking at those:
c2pa_sign_aws_kms I have no idea why this one exists, it's just doing the same thing it is doing in requirements.
acquia_connector is valid and they would want to maintain that even during updates if they are going to provide older versions of PHP. I think we need the alter.
schemadotorg_metatag while this runs during update, not sure it needs to.
gin_toolbar while this runs during update, not sure it needs to.

I think the acquia connector is one that would need to run in the update alter.

> I think the acquia connector is one that would need to run in the update alter.

Actually not, no. system_requirements() already deals with runtime update, again to not prevent you from updating:

$requirements['php']['severity'] = ($phase === 'runtime') ? REQUIREMENT_ERROR : REQUIREMENT_WARNING;

It will never be ERROR on phase update, only on runtime.

Also, this will all only be relevant again in 2028. The logic around this is that we switch to a runtime error if the PHP version is EOL, per \Drupal\Core\Utility\PhpRequirements::$phpEolDates, that is 2027-12-31 for PHP 8.3. Nobody will see a requirements error for the next 3 years.

But either way, I'm OK with keeping the alter hooks just in case, probably not worth to spend too much time on discussing that (oops, too late).

Some good discussion. And I like how this has good. #61's organisation does look good.

+1 to not doing hook_requirements install phase in the initial work. As mentioned this whole thing is flawed. We need to get away from running code from modules that are not installed. Imo we should do something like check in a module (or sub module) has a composer.json and validate it's dependencies - this would probably remove most of the current usage and could even do some of the conflicts stuff. Let's open an issue to work on this.

I think it's here: #2494073: Prevent modules which have unmet Composer dependencies from being installed →

FYI: Both update and runtime are RTBC. There will be a merge conflict with the 2nd one after the 1st one lands, since both are touching core/lib/Drupal/Core/Extension/module.api.php -- it'll be a trivial fix, but it means a core committer cannot do both simultaneously without one of us doing a manual rebase in between.

If we wanted to arbitrarily pick one to commit first and mark the other postponed, that'd be okay. Or we leave both RTBC and resolve it organically.

Thanks!
-Derek

Assigning credit here for all the discussion. Given the two issues are RTBC and we've agreed an approach for update and runtime I think we can marked this as fixed once those are in. Install discussions can continue in 📌 Create class to replace hook_install_requirements Active

As both 📌 Create class to replace hook_update_requirements Active and 📌 Create hook_runtime_requirements Active have landed going to mark this as fixed.

@alexpott: there is still phase 2 and 3 to consider, no? (converting all implementations and triggering deprecations)

Thanks for merging those!

I think we can leave this Fixed even with phase 2 and 3.
I actually think it makes more sense to move install discussion to 📌 Create class to replace hook_install_requirements Active
Then we can convert 📌 [pp-3] Convert hook_requirements to new classes or hook_runtime_requirements Active to a meta for tracking what changes we're going to make to existing requirements.

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