Problem/Motivation

Hook requirements uses phase right now to manage whether it is runtime, install, or update.

This leads to significant complexity. As it stands hook_runtime can easily be an OOP hook.

Let's create a new hook_runtime_requirements() so we can start moving those.

Steps to reproduce

N/A

Proposed resolution

Create hook_runtime_requirements()
Create hook_runtime_requirements_alter()

Invoke hook_runtime_requirements() after hook_requirements()
Merge results
Run hook_requirements_alter()
Run hook_runtime_requirements_alter()

Remaining tasks

None

User interface changes

N/A

Introduced terminology

API changes

hook_runtime_requirements()
hook_runtime_requirements_alter()
Can be implemented to provide our alter requirements on status report.

Data model changes

Release notes snippet

Issue fork drupal-3490841

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 !10410

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
nicxvan’s picture

nicxvan commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record updates

Think the CR should be updated with some code examples

Also some test coverage for the hooks to show they work.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
Issue tags: -Needs change record updates

Updated both.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Didn't push up the alter test.

nicxvan’s picture

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

nicxvan commented
Issue tags: +11.2.0 release priority
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

I don't think we need to convert all here but could we convert least 1-2 instances to use this new hook. With the test coverage I do believe it's working but it being used live would be great.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented

Appears to have a pipeline issue. But keeping an eye out for this one.

smustgrave’s picture

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

Thanks for adding that example!

Rest looks good to me

dww’s picture

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

Thanks for this! Exciting.

I’m only looking at the MR on my phone. I have some concerns. I’ll post a thorough review later when I’m not thumb-typing.😅 But I don’t believe this is totally ready in its current state. No rush, since this isn’t happening for 11.1.x. But getting out of the RTBC queue for now so committers don’t waste time on it.

dww’s picture

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

Or I guess to make @smustgrave happy, NW is better. 😂 Sorry to change the status to that without explanation, but I’ll open MR threads later with details.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I'm not sure about the decisions that are creating this API. It feels like the question is how can we make this an OOP hook - and not what is the correct API for gathering requirements during install, update and runtime. The solution being advocated for in this and the other issues feels just asking ourselves what is possible with OOP hooks - and not what it is the correct API for gathering requirements from modules.

nicxvan’s picture

nicxvan commented

I'll reply on the meta.

nicxvan’s picture

nicxvan commented
Status: Needs work » Postponed

Marking this postponed since an alternate approach was suggested in the Meta

nicxvan’s picture

nicxvan commented
Title: Create hook_runtime_requirements » Create hook_runtime_requirements and hook_runtime_requirements_alter
Status: Postponed » Needs review
dww’s picture

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

The MR thinks the merge is blocked due to conflicts. Would you be willing to rebase? I just did a thorough review on the sister issue. Probably many of those nits would apply here, too. ;) I'm sadly out of contrib time for today, so I'll have to come back to this later for a more thorough review of the details here.

nicxvan’s picture

nicxvan commented

Thanks! I can rebase and transfer the feedback here too tomorrow.

nicxvan’s picture

nicxvan commented

I applied all feedback from #3490842: Create hook_update_requirements() and hook_update_requirements_alter()

nicxvan’s picture

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

nicxvan commented
Related issues: +#3490842: Create hook_update_requirements() and hook_update_requirements_alter()
smustgrave’s picture

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

Believe after this rebase this one is good to go.

Re-ran the pipeline and all failures were random.

CR reads well with the examples and conversion in the file module believe shows it works in real time.

dww’s picture

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

Needs rebase after #3498059: Add array return to all hook_requirements implementations landed. Also, added a bunch of suggestions to cleanup the API docs.

Initial pass at saving credit: @nicxvan for all the work, and @smustgrave and myself for reviews.

nicxvan’s picture

nicxvan commented

I think the failure is from upstream, nothing here would make the navigation stylesheet larger.
I think it may be here #3425081: Integrate Navigation with Workspaces

I pushed up another change to a comment and the same job is failing.

It's possible I made a mistake rebasing, but the changes look right and the performance test assertion matches HEAD.

I bumped the value here so it passes, but also created #3500293: Add headroom to the navigation performance test

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
dww’s picture

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

Edit: whoops, wrong issue.

dww’s picture

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

This, too, might be RTBC, but lemme do it right if true.

dww’s picture

dww
we/he/they
commented
Title: Create hook_runtime_requirements and hook_runtime_requirements_alter » [PP-1] Create hook_runtime_requirements and hook_runtime_requirements_alter
Issue summary: View changes
Status: Needs review » Postponed

Yeah, I don’t know if the quick-fix here in an unrelated test is the right approach. I believe we should postpone this on resolving #3500293: Add headroom to the navigation performance test

dww’s picture

dww
we/he/they
commented
Title: [PP-1] Create hook_runtime_requirements and hook_runtime_requirements_alter » Create hook_runtime_requirements and hook_runtime_requirements_alter
Issue summary: View changes
Status: Postponed » Needs work

Blocker is in. This needs a rebase and to revert the last commit.

nicxvan’s picture

nicxvan commented

Yep I'll take care of it

dww’s picture

dww
we/he/they
commented
Title: Create hook_runtime_requirements and hook_runtime_requirements_alter » Create hook_runtime_requirements() and hook_runtime_requirements_alter()
Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll

Phew! Thanks for sticking with this through all the rounds of reviews. Apologies I didn't spot everything all at once.

Everything is ready here now. Pipeline is green. Code and docs have been polished so much they shine. 😂 No other improvements I can spot. Per the meta, these separate hooks really are the right approach for splitting up the beast of hook_requirements().

Ready To Be Committed. 😉

dww’s picture

dww
we/he/they
commented

p.s. Did a light edit on the CR for formatting and pedantic nits. 😂 https://www.drupal.org/node/3490851/revisions/view/13843726/13854897

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 5e0a4ce and pushed to 11.x. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

  • alexpott committed 5e0a4ce7 on 11.x 
    Issue #3490841 by nicxvan, dww, smustgrave: Create...

quietone closed merge request !10410

Status: Fixed » Closed (fixed)

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