[pp-1] Review requirements duplication

Copying some relevant comments from the MR threads...

dww

Looking more closely, this whole method (in core/modules/pgsql/src/Hook/PgsqlRequirementsHooks.php) is identical to core/modules/pgsql/src/Requirements/PgsqlRequirements.php. Any harm in this runtime/update Hook instantiating the special install-time class to re-use the code instead of maintaining 2 distinct copies of this?

nicxvan

The line of what can be injected is different for install phase. So it feels safer to keep it separate, there is a small amount of duplication, but I can create follow ups to explore, this issue is more about just removing the hook_requirements.

dww

I know what's permitted to inject is different, which is why we have the whole separate class mechanism for install time. However, if the Install object is safe at install time, with less code loaded, surely it would work to use it at runtime with everything loaded

PackageManagerRequirements has some minor duplication as well.

Re #3: isn’t that 📌 [pp-1] Review duplicated requirements check in package manager Postponed ?

yes, thank you!

Let’s be more specific so we don’t confuse ourselves. 😅

The blocker is in, I think a static method is the best approach if possible

@larowlan: What's wrong with my plan to just instantiate the install-time object in the runtime OOP method and call it directly? Why does it have to be moved into a static method?

We might have other chances to establish the "state of the art" in this space over at 📌 [pp-3] Split up and refactor system_requirements() into OOP hooks Active , but this is a much simpler initial case to tackle. 😅 Personally, I think it's cleaner to instantiate the install object and use it at runtime than it is to refactor both to rely on a static method. Whatever DI hurdles / restrictions the install time object must conform to means it's a subset of what would be allowed at runtime, so I don't see how it could break.

Thoughts?

Thanks,
-Derek

p.s. Are we sure this is "Novice" material? This stuff is pretty thorny, with some tricky bits to get right. Not sure we want this to be someone's "first contribution to core", etc...

@dww not fussy either way, if we can cleanly instantiate it - works for me. Removing the Novice tag

Cool, thanks.
More accurate title. 😉

Oh hah, I forgot that InstallRequirementsInterface::getRequirements() is already public static. 😂

Working on an MR, stay tuned.

Pretty simple, other than 📌 Move REQUIREMENT_* constants out of install.inc file Needs work . Pipeline is green on both MySQL and PgSQL.

Rebased after 📌 Create enums for RequirementSeverity and RequirementPhase Active landed. Removed the yucky now that we've got an enum instead of needing to include install.inc.

Seems pretty straight forward. The same code is in PgsqlRequirements so this seems like a clean replacement.