Just one final comment that I think needs addressing.
My questions were answered I'll do a more thorough review later unless someone else gets to it.
Did a quick review, a few questions where I feel like I'm missing something obvious.
Unfortunately lots of failures that seem relevant, have not reviewed any.
Done
The coding standard issue is RTBC
I think with three supporters we can RTBC this.
I updated the IS with the new language.
Thinking about your feedback on:
if (!defined('MARK_READ')) {
vs
if (!in_array(\DRUPAL_ROOT . '/core/includes/theme.inc', get_included_files())) {
This check is needed for just one test where preprocess functions are not loaded yet.
This whole stanza only gets hit during site install so there are no performance implications there.
However during tests every time drupal is installed we'll hit this and have to check.
It's a small thing, but I think checking the const is probably less intensive than the get_includes check. They both accomplish the same thing. Is it worth changing that back?
Addressed your feedback, left a comment on one.
Applied three and responded to one comment.
nicxvan → changed the visibility of the branch 3487957-convert2.0 to hidden.
@andypost, I don't think we can add that until https://www.drupal.org/project/coding_standards/issues/2999367 ✨ Require using a colon after case instruction in switch statements Active is resolved.
This fixes the current inconsistencies, then we need that other issue, can you review that one? I think it has the supporters needed.
I added a procedural test too.
Ok this is ready for review.
I ran the test only, but that won't fail cause the current code and previous code both work for the situation, but it will prevent the regression.
I'm taking a crack at this.
Having worked with this for a bit, this isn't the right issue for what I mentioned in 42.
I'm doing a review now.
If you extend it and set it as a decorator the original event listener tags move over as well.
Also, yes if multiple interact it would probably get messy quick, but in order to prevent that what do we lose?
I've refactored this to be closer to the way it originally worked.
The one change is now the installer loads only the minimal needed to work.
Refactoring a bit
Yes that is what it should look like.
Oh yeah we also have to get rid of hook info and replace hmia too.
Yes 11.x
Since that is the version selected in the issue now if you click create new branch it should auto select that one to cut from.
Just commenting to say it's simple to change php or even mysql or mariadb versions in ddev too.
I can't edit it for some reason.
Target is still wrong
Not sure if I agree with marking hook classes as final.
Until we have a proper way to handle replacement long term I think it's better to leave them not final.
Looks good to me, all cron hooks have : void.
nicxvan → changed the visibility of the branch project-update-bot-only to hidden.
nicxvan → changed the visibility of the branch 3488176-convert-systemtheme-to to hidden.
Let me test something
Just to clarify, whether those code style changes are acceptable here is up to the maintainer. It's not an inherent issue.
If you're ok with addressing code style issues here too then we can review it as is.
- Bot one is 43.
- Mine is 59.
- Newest is 62.
If we do not want to remove the system theme hook entirely then we will likely need to special case the install and load what is needed for the installer to work.
It might be more desirable to approach it this way cause then system_theme stays the same but it's OOP.
The issue is two fold.
1. The class is not loaded during install so the OOP hook will not be there. The procedural hook is still picked up in that situation though.
2. The RegistryTest does some funky stuff and Drupal is not fully there during that test.
What is in this pr bypasses those two issues, but we no longer have a system_theme hook implementation at all and we're loading the templates somewhat manually.
I created a new one because I don't like pushing to the project bot only branch and we had to rebase cause the paragraphs issue also created the composer.json.
I'm happy to close mine.
Looking briefly at the new one, it has a lot of changes unrelated to drupal 11 support: 43 files + 624 − 661.
Whether that is acceptable is left up to the maintainers, but that is a lot more to review than the changes in my MR, though my MR still needs the jquery dependency issue resolved.
Let me know if this requires a CR.
Ok let me update those.
The current issue seems to be that for some reason $this->moduleList->getPath('system') does not return the path to system, so when the preprocess hook process runs it doesn't have the path to the templates.
So then later on when they are diffed $diff = array_diff($parent_hook['preprocess functions'], $cache[$source_hook_name]['preprocess functions']); it fails on container since the templates have not been crawled.
At least this is what I think is happening.
Do you just want the class comments updated?
How about something like:
This class will not have an effect until Drupal 11.1.0.
This class is to prevent phpstan errors for modules that support procedural and #Hook implementations.
nicxvan → changed the visibility of the branch 3487971-convert-remaining-two to hidden.
I'm going to split system_theme out of this cause that has gone a bit far afield.
Each baseline item is 6 lines too, that means you've eliminated over 4800 lines from baseline with another 3000 to go!
@berdir is mostly correct. The one additional thing I might consider here are child tasks to eliminate module .inc files.
They used to be useful from a pure organization standpoint, but now many have just a couple preprocess functions and with the .module files so much shorter it may make sense to consolidate some.
This title may be better called something like evaluate core .inc files in modules.
Looks good!
Great catch on the ones that returned values.
To answer my own question it is about the return of hook theme.
I prefer TemplateDefinition over TemplateImplementation
The root of the issue for system_theme is that when Registry is called during install processExtension doesn't register system_theme as a hook when it's an object, but it does for the function even though the invoke call doesn't use the legacyInvoke fallback.
I then moved it to a static method so I could add it when needed, and combined system_theme and drupal_common_theme since that was the only call to drupal_common_theme (though that might be like that to load the constants in theme.inc)
Yeah, this was cause there are some wider things for deprecating hook_hook_info. This might become unnecessary though.
I might split out the page attachments from system_theme
Page attachments is pretty straightforward.
system_theme is getting a bit complicated I fixed the install process, RegistryTest will likely fail still.
I imagine it needs some discussion.
For some reason some use statements are being duplicated.
nicxvan → changed the visibility of the branch 3487957-convert-to-oop to hidden.
Also, if we want to prioritize .install etc, there is a path we can take where we just have a special serviceProvider like class for the install time ones, we don't get the true benefits of OOP, but it would still give some organization options and we could deprecate .install files at that point.
I think these are the ones in .install still:
* - hook_install()
* - hook_post_update_NAME()
* - hook_schema()
* - hook_uninstall()
* - hook_update_last_removed()
* - hook_update_N()
These are sometimes in .install, but they already support OOP
* - hook_module_preinstall()
* - hook_module_preuninstall()
* - hook_modules_installed()
* - hook_modules_uninstalled()
Once that is broken down it would make it easier to scope out how to truly convert those hooks.
The really big hurdle I think just from an amount of code in .module files is handling:
- hook_preprocess_HOOK
- hook_process_HOOK
- hook_theme_suggestions_HOOK
As well as handling hooks called from themes, if we solve those two things that covers like 80% (made up percentage) of the remaining code in .module files I think, most everything else in my experience are helper methods that are in .module for convenience and can be moved, or things for batch / queues, but those already have a way to move.
Hook collector picks up all functions that start with module name.
Not quite, but close.
I think we need to figure out follow up 2 here: https://www.drupal.org/project/drupal/issues/3472165 📌 [PP-1] Explore converting Install hooks to OOP Active
Then we need to figure out how in themes / on behalf of themes.
Hook hook info
Hook module implements alter
And where to put cost and define statements.
Then we can.
I think we're close to deprecating .inc
We just have to figure out the three helpers for views and Hook Hook info for that I think https://www.drupal.org/project/drupal/issues/2233261 📌 Deprecate hook_hook_info() Active
If you click on it and search for fail you can find the failures.
It's layout builder which is a known common failure.
If you click get push access you can run that test group.
Also, only slightly related, but I think I'm going to create a cleanup meta now that core is converted to clean up some inc and module files that only really have constants or a single helper function.
📌 Deprecate Procedural hook implementations Active is a super naive implementation of this. Chx pointed out this is to expensive to really do it this way most likely.
I plan on cleaning up my conversion script for contrib over the next week or so.
Tests are passing in XB with this change now. https://git.drupalcode.org/issue/experience_builder-3487815/-/jobs/3373246
Not unless there are other issues brought up, but it might be a while for a few reasons so please be patient.
First we're in the beta window so the core team is focusing a lot of efforts on some initiatives around that.
It can sometimes take a few weeks for an issue to get merged in.
I created 📌 [ignore] Test Filecache Active we can apply this there
These changes look good to me actually, can we test this against XB before going through the commit process again?
Just a quick note, you can implement on behalf of other modules using #[Hook('toolbar_alter', module: 'module_on_behalf_of')]
You can't implement on behalf of a theme though.
johnpicozzi → credited nicxvan → .