Create class to replace hook_install_requirements

Makes sense to move this to a hard-coded class instead of a hard-coded function name, I don't there's anything else we can do in install.

However the MR is missing some hook_requirements() which affect mutliple phases, such as in package_manager_requirements()

That was considered, I'll update the meta to add more details, but here is the gist.

We replace the $phase = 'install' portion with this issue.

We replace the $phase = 'update' with 📌 Create class to replace hook_update_requirements Active

And we replace the $phase = 'runtime' with 📌 Create hook_runtime_requirements Active

Once we have all three this covers most cases, but for more complex hook_requirements like system and package_manager with multiple phases we convert in a follow up.

Once core is converted we deprecate hook_requirements being called without LegacyHook being attached.

> Makes sense to move this to a hard-coded class instead of a hard-coded function name, I don't there's anything else we can do in install.

The problem with these hooky-classes, like this and ModulePascalServiceProvider is that unlike hooks, we have no standard for documenting them.

We can document it on the interface, which is where I put it, it definitely needs some fleshing out, but it's there.

If you mean the implementation side then we can just document the classes implementing it.

That's good, but we'd still be losing the discoverability of it in the documentation as an API.

With hooks, there is a @hooks topic that explains the concept and lists the API elements. With HookyClasses, you only learn of a particular class if you happen to stumble on it.

Oops, didn't mean to change status.

Though I'm confused why the other two replacements of this hook are hooks, but this one is a class? What's the difference?

I still think 18 is a separate issue and not a blocker. For 19 there are two reasons:
1 the other two are invoked by module handler so we can just change the hook.
2 this one happens during Drupal or module install is to early.

This is ready to review again, the concerns on the meta have been resolved.

> I still think 18 is a separate issue and not a blocker.

Every time we say this we are eroding the quality of our documentation. Each time it's a small separate issue and not a blocker, but these keep adding up.

I'm happy to work on wording in a sibling issue if you create it, I think it would belong in a subpage here: https://www.drupal.org/docs/getting-started/system-requirements →

Applied some nitipicky stuff but reviewed the rest of the changes and nothing seems off to me.

Thanks for the CR updates we mentioned in slack.

I did open 📌 How to best handle documentation around new OOP hooks Active as a child of the META and tagged it as a 11.2 release priority.

I think we should not be doing this. Creating another way to load code from a module that is not installed feels like we are perpetuating the problems of hook_requirements() with the install phase.

Added some comments to the MR. But I think we should explore reading the modules composer file and check that instead of doing this.

Those are good ideas I can update this later.

I still think we should do this for the reasons stated in in the meta, particularly it's easier for people to know each has a phase somewhere to go. We can mention in the CR that it is preferred to use composer and a link to the issue to watch, and that this is intended as a bridge.

Ok I addressed all of your feedback I agree it looks cleaner.

I did copy the class load from the drupal_check_profile, is that a separate concern?

Setting it back to needs review because I addressed the feedback and it's green.

I'm going to update my comment on the meta.

> I think it would belong in a subpage here: https://www.drupal.org/docs/getting-started/system-requirements →

Putting some documentation on d.org is great, but that's still a worsening of what we have. Hooks are in-code, and most importantly, *systematic*.

You look at this:

 * Implements hook_requirements().

and hook_requirements() is obvious as a thing that you can look for. API module handles it automatically to link up the specific instance to the general documentation for the component.

You go to the hook_requirements() documentation, and you see it's in the @hooks group. API module makes this a link.

You go to the @hooks group topic, and that explains what hooks are as a type of extension point. API module lists all of the hooks that exist.

But with something like:

class MyModuleServiceProvider

we have currently at best an interface. It doesn't tell you the class must be magically name, and it doesn't connect to the other extension points that behave in the same way.

As a counterpoint, I think it is far more standard to look at the interface something is extending and have the documentation there.

That documentation tells you that it must be magically named.

The implements piece is something the community is used to but you either have to leave the ide to find docs or you have to know to search for function hook_requirements otherwise search results are flooded with other implementations.

Also implements comments can be wrong or missing, extend can't be.

Again finding other implementations of a hook either requires leaving your ide, knowing regex, or the implementors perfectly following the naming scheme for a comment.

Finding classes implementing an interface is mostly built in to ides and something people beyond the community are used to too.

I agree having docs outside the ide is helpful which is why I suggested that docs pages, other suggestions are welcome.

If you're looking for a way to document classes implementing an interface the issue smustgrave created is probably the place to discuss that process and what is needed. It should probably be moved to api and retitled to focus on this use case since we already have one for hooks specifically.

@alexpott, I addressed your feedback and created the followup here: 📌 Remove class_loader in drupal_check_profile Active .

I agree that it would be nice if we can get rid of most* install requirements hooks but that requires multiple new features (other modules, extensions, composer dependencies, conflicts), not all of which can be composer (we can't use composer conflict because we can not and don't want to prevent them being present, it's about them being installed or not).

hook_requirements() is the last remaining "regular" hook (there's just preprocess and special install/update stuff then). If we have that and preprocess done we can drop that stop parsing attribute and so on. modules are either fully converted or not, no weird middle ground.

In other words, it would be very nice to find a way to deal with it. I was thinking we could just keep hook_requirements with phase install for now until we see what's left once we improve other things. But I think that's hard to deprecate and detect allowed implementations vs not, we can't introspect what $phase it has and what it does with it.

On the class name: Do we really need to prefix it with a camel-cased moduel name? Yes, we do it for ServiceProvider, but it's namespaced anyway. Yes, we'd have 5,10, ... "InstallRequirements" classes, but is that really a problem?

That's the only part I agree with from #31. Everyhing else I very much don't. As #32 said, what #31 describes is like 4 different Drupalisms that nobody else uses and new developers don't know about. Nobody except Drupal uses a .install file. Nobody else has an api module, api.drupal.org only provides info on core, it requires a Drupal-aware IDE like PHPStorm or knowing what to search for to maybe find the hook_requirements() documentation and understanding the *magically named* hook functions. Now you need to look at src/ (where _all_ code will be soon enough), where there might be an Install folder and then a Requirements class inside. Looking up the interface is standard PHP stuff, documentation on that is standard PHP stuff. To me, that is way better DX, especially for developers that don't know Drupal. And we want and must attract new developers.

That said, if we're not sure yet if this has a future, we could also just rename it to hook_install_requirements(), but then we have to explain that this is very different from the other two split requirements hooks. I'd prefer a class but I don't care too much. What I do care about is being able to forget about as soon as possible.

On the class name: Do we really need to prefix it with a camel-cased moduel name? Yes, we do it for ServiceProvider, but it's namespaced anyway. Yes, we'd have 5,10, ... "InstallRequirements" classes, but is that really a problem?

Yes, we cannot autoload it so there is no way to find it without knowing the naming convention unless you have a different idea.

> Yes, we cannot autoload it so there is no way to find it without knowing the naming convention unless you have a different idea.

My naming convention suggestion is only and always name it "InstallRequirements", nothing dynamic. It's namespaced, it doesn't need to have a prefix. Downside is that there will be a bunch of classes with that name, but the long term plan is removing all/most of them.

I think prefixing keeps it closer to the service provider model and makes it easier to find the one you want.

Yes removing them is the long term goal, but finding a specific install requirements in a list of 15 will be extremely painful.

232 commits behind 11.x. Rebase.

I'll fix it, the test names are wrong.

Pipeline green. Test-only fails as should.

All threads seem resolved. RTBTC?

Reviewed the MR and don't see any open discussions or tasks. Believe this one should be good to include in 11.2

Thanks for working on this! Working on an MR review. About to open a bunch of threads. But moving out of RTBC queue for now so a core committer doesn't spend time looking just yet...

I agreed with most of your comments, I had to resolve most of them so I could set up my own suggestion though, e.g. I renamed module to extension.

I honestly think naming them all the same would be a huge DX problem.

As for the requirements constants, I think we address that in the follow up to handle those oddities.

I wonder if this will work #2910814: deprecate magic ServiceProvider file discovery; declare in services.yml →

Don't want to overstep @dww what do you think with the feedback?

Yeah, I think we need him to weigh in.

#2910814: Deprecate magic ServiceProvider file discovery → won't work for this situation cause we need it for uninstalled modules.

What if we do a small discovery in the install namespace, then it can be named whatever as long as it extends the requirements class.

Ok, how about MR !11218 mergeable?

Now the only restrictions are that it be in the Install namespace and that it subclass InstallRequirementsInterface

I think that is acceptable and there is no magic naming whatsoever so we can name it unique so it can be found.

@nicxvan thanks for the new MR.
I prefer new approach, I left some feedback, please check.

Thanks for your review! I applied two and replied to the other one.

One question is if the namespace should be Install/Requirements though so we can have other install time classes.

Ok I moved it to Install/Requirements to give us more flexibility if we need to add more install time hooks in the future for organization.

I also added a check for the install requirements text due to a suggestion from @ghost so we don't blindly require code.

@nicxvan thanks for update, MR looks good. But please update comments as well.
I would not change status, because this is not critical, and it would be great to get more feedback.

nicxvan → changed the visibility of the branch 3490843-create-class-to to hidden.

I hid the other MR.

I also updated comments and the CR.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Rebased and also addressed @berdir's last concern.

I think this is ready to go back to RTBC.

Yes, executing code pre-install is icky, but it's not new and this is one of the last blockers to deprecate legacy hooks for good. Which I think has a bigger benefit.

There are multiple existing issues to implement support for checking composer dependencies, conflicts and other common scenarios, which will hopefully remove most although probably not all use cases for this.

for system_requirements(), I think we can still consider to remove the "drupal install requirements" into Drupal\Core\Install or something instead of stuffing it into the system module, but we can do that in a separate issue.

Committed c3e3008 and pushed to 11.x. Thanks!

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