For a start this will likely need framework manager review.
Great question!
This is somewhat anecdotal, the test run here was 6 minutes.
I checked 4 other issues they all ranged from 8-15 minutes.
Left one other question.
You don't need to test deprecations directly unless there is complex BC logic.
The question is what happens if someone is calling that function directly and the files get loaded in composer as well.
Thank you, I'll review that linked issue when I have more time.
I didn’t want the non-stable color to be too much of a warning. While we do hope production sites can run as many stable releases as possible, we also want maintainers to have some visibility for testing pre-releases.
This is less about the non-stable being more of a warning than the stable one being more easily seen as recommended. Blue doesn't read as stable to me.
The second and related issue is the grey is harder to see, but really the grey and blue look very similar to me. When they are side by side I can see the difference, but on modules that only have one or the other it's very hard for me to tell if the module is stable or not.
Will this affect unit tests?
If I understand correctly the legacy files would not be loaded before this change, but after this change all unit tests will load all legacy includes.
Not sure if this is a problem or acceptable, but it likely is a change.
Yes, this needs a rebase, the other issue has the same failure.
Yes, this needs a rebase, the other issue has the same failure.
Ok I've pushed it up, I've attached a screenshot of the before and after
12 and 19 still haven't been addressed, I'm working on this now so assigning myself since there have been a few contributors recently.
That might be the long term fix, I meant something more short term, like throwing an error if numeric keys are found.
That is better!
I added a suggestion for that comment.
Rebased and pulled in node and update requirements changes.
nicxvan → changed the visibility of the branch 3500632-convert-hook-requirements to hidden.
I'm not sure this is the place for it, but is there somewhere we can make this more robust to prevent this issue in the future?
When it's for phase runtime or update they are hooks still so those go in the Hook namespace.
If they are install time then that is not a hook, it is a special class, the change record is here:
https://www.drupal.org/node/3492429 →
Explanation is great, fix is simple, there is one test that looks like a random failure I cannot rerun.
If that passes I think this is rtbc.
Good point! That makes me lean towards publishing it.
So it would already be doing something deprecated.
Not quite, procedural hooks are not deprecated yet.
But I struggle to see the use case where one would have both an oop and procedural for the same hook in the same module.
Had a couple of questions.
I went through very carefully and confirmed all of the replacements align.
Two minor changes.
I think the option to have the custom message is nice.
There was a failing test, but it looked random.
Core uses merge requests so please convert the patch after you confirm the steps to reproduce.
I just want to know what the banana is.
@hadi farnoud the good news is you can work on this if you still think it's valid, there is a create issue fork button at the top.
If you press that it will create a fork you can begin.
There is a patch to start with, but it's 13 years out of date.
Does this help 💬 Drupal 11.2 upgrade causes \Drupal::$container is not initialized yet error Active
Does this help 💬 Drupal 11.2 upgrade causes \Drupal::$container is not initialized yet error Active
Yeah I meant more of a concrete example, I mean it really feels like overkill to load HCP for just this one bit.
Still not ready to pull this out of RTBC, but it feels like the wrong place for this.
Hiding files, I converted it to an MR.
I think catch's involvement means we can remove the framework manager tag.
After some back and forth on slack the confusing bit has been clarified on why it's inverse.
I think this is unfortunate but necessary, and it's only temporary since the upstream issue was committed so quickly.
This is pretty close I think. I have one suggestion for the comment describing what is happening.
I have one clarifying question that I couldn't quite understand.
One comment commiserating on the actual workaround bit lol.
Not a fan of final, it's already marked as internal which should be sufficient. I wouldn't block this issue on that opinion since it's not settled.
Can you share an example where you're trying to use this?
I think this issue might finally be ready!
My recent commits have all been documentation and test organization related based on direct feedback.
So while I've been heavily involved in this isue I think I can RTBC for the following reasons.
The functional pieces I have written have long been reviewed and approved.
This issue has had significant feedback and it has all been addressed: (210 comments on the issue and 849! on the various MRs)
The developer facing api, scope and deprecations have been reviewed and approved by @larowlan and @catch.
The sticking point for those reviews an many others were the extra types bit in the order before and after.
The underlying implementation in modulehandler and hookcollectorpass was rewritten by @donquixote to remove the extra types concern entirely and close the gaps introduced by extra types.
@ghostofdrupalpast and I have both provided deep reviews of those rewrites.
@donquixote also provided additional testing beyond the tests reviewed in the initial approach.
@quietone provided a full review of comments and api docs.
@godotislate and @berdir contributed to feedback on early implementations.
@amateescu reviewed the conversion for workspaces and identified a gap in the implementation that was fixed.
All in all, I think all concerns have been resolved.
The pieces I wrote have been reviewed by several others, the pieces I have not written have been reviewed by myself along with others.
I think this is ready.
Am I missing something out was this a feature request to start?
The history shows this issue changing to a support request in 2.
Quick comment on the constructor.
Any reason behind the file and function names being inverted?
Creating stubs for the follow ups.
nicxvan → created an issue.
I reviewed your contribution in https://git.drupalcode.org/project/drupal/-/merge_requests/11796/diffs?c... and that looks great to me.
I also applied your remaining suggestions and we're now a lot slimmer.
I think this is ready again.
That's on the roadmap but this is only about template preprocess hook after 📌 Explicitly register template_preprocess callbacks in hook_theme() Postponed
Here is where we are working out the naming convention: 📌 [meta] Clean up hook classes in core Active
All of the feedback has been addressed!
This is ready again.
Yeah no objection here.
Happy to have the help!
I'd vote for splitting it along the 4 lines you mentioned.
Let's move them to ${Module}ThemeHooks, having files called the same is already causing issues finding the right file.
Let's hold off on splitting the last chunk until we get a look at it, but if anything obvious pops up we can split those out to a new issue pretty easily.
That makes more sense, I've pushed that up!
I've rebased this now that 📌 Explicitly register template_preprocess callbacks in hook_theme() Postponed is in.
Ready for review again!
I've also modified this to remove the changes that spawned
📌
Explicitly register template_preprocess callbacks in hook_theme()
Postponed
e.g. this no longer captures template_preprocess_hooks in hook collector pass.
nicxvan → changed the visibility of the branch 3495943-preprocess-combined-approach-no-template to hidden.
I updated the CR for clarity, I agree this is RTBC again.
It will need to be manually replicated.
I think all of Catch's comments have been addressed!
Leaving the direct call as an example I think is fine for now. I confirmed the remaining items are all in html and page and agree it wouldn't improve anything by further splitting.
That works for me!
Oh I see how that was trickier to test now.
Test only fails as expected.
I don't think this needs an change record.
Seems pretty straightforward.
This has been rebased on top of the changes in the define template preprocess hooks explicitly.
I also undid the template module key and conversions. This only allows for converting preprocess hooks and hook_preprocess in modules.
nicxvan → changed the visibility of the branch 3495943-preprocess-combined-approach-with-skip to hidden.
Removing tag since this approach was approved as long as the template bit follows 📌 Explicitly register template_preprocess callbacks in hook_theme() Postponed
This will need to be modified for that issue, but I'm not postponing since it's such a small part of this issue and it can still be reviewed.
This might be better defined as a feature request.
Yeah, I was hoping for some code to look at to see where it could get hooked in.
Can we get some concrete examples?
I confirmed it is only in tests.
It's simple enough, I did not see any other instances.
Adding 11.2.0 release priority since the preprocess hook issue now depends on this one.
Thank you for your review @quietone!
Where in module.api.php would we put this documentation? This is related to hooks, but it's more about ordering than defining a hook so I'm not sure where it would go.
I think most of your other comments make sense, we've replied to a few.
Regarding #199, we chatted in slack and agreed this issue is now stable. We'll manage any review feedback, but hold off on further refactors beyond that.
I think this is ready for full review and we will work on integrating the feedback @quietone provided so far.
We are working on over hauling preprocess functions to be oop for modules at least, it may be an nice follow up to check for the hook attributes once that is in.
I love that you left the rest of the title lol
I think this is great, do we need something more generic long term?
I can update the crs to be more explicit.
https://www.drupal.org/node/3499788 → refers to both issues because neither have landed, whichever lands first the other reference will be removed.
I'll update the crs shortly.