This might be related.
@berdir created this 📌 Explicitly register template_preprocess callbacks in hook_theme() Postponed on top of 📌 Move template_preprocess, _template_preprocess_default_variables into services Needs work as an alternate to how we are handling template_preprocess and template_preprocess_HOOK in this issue.
It does include a BC break if we do it that way, but we could include it in this issue if that is preferred and add a deprecation for template_preprocess_HOOK calls in a follow up pretty easily.
We discussed this briefly on slack, so I'd like to move some thoughts here.
#3495943: Handle module preprocess functions as OOP hooks is struggling a bit with that and need to introduce the ability for modules to register them as hooks but not for themself but a "template" pseudo-module.
I'd disagree with the characterization that it is struggling, it is solved. I'm not sure I'd call it a pseudo module either, it's a prefix, it's basically the same thing Registry does for it.
As for this issue:
I think it's an interesting approach
use\Drupal\Core\Utility\CallableResolver to also support services.
I think we'd definitely want to do this, it would be a shame to have DI for other preprocess hooks but not these ones.
This does make theme_registry_alter harder to maintain BC for contrib which is not a problem for the original issue, but I'm not sure how many people are moving template preprocess around, and I'm not sure this should be a blocker for this approach, just an observation.
Another thing I see is this relies on call_user_func_array for calling the initial preprocess, I am not a fan of that call, the original approach only uses it for fallback for custom callbacks added in theme registry alter.
If it's decided that this is the direction to go, I really think this should be done together with the other issue, this isn't too much more and if we pull out the template work in the other issue the changes are likely a wash complexity-wise.
How would we go about using this to test?
Sorry, I was basing that question off of your comment about duplicate events, does adding the item trigger the call a second time?
So it works like this
Add item A to cart
ECA triggers
Add item B to cart
ECA triggers
Set item B to quantity 1
User then increases quantity to 2 on item B.
Add item C
ECA Triggers
Set item B to quantity 1
I'm honestly kind of stuck between adding this in the simple form you have and complicating it for a couple more scenarios.
Maybe we need two more checkboxes:
Checkbox 1 is multiply whether to multiply item B quantity by the quantity or just set it to the value.
E.G. if the rule is if Item A is added add 2 of Item B, if you add 2 Item A should you get 2 or 4 of item B.
Checkbox 2 is override whether to change the quantity of item B if it is higher than the quantity specified by the rule.
E.G. if the rule is if Item A is added add 2 of Item B, if you add 2 Item A and you already have 5 of Item B should it be brought down to 4 or left alone.
Simplified the patch a bit.
Just made it match the setting.
nicxvan → changed the visibility of the branch 3325268-vimeo-video-autoplay to hidden.
If this solution worked for you please post a response so we can add it to docs.
Yes, this is possible, if you have completed this please reply with your steps so we can add it to docs.
This looks like a great addition! Can you please take care of the PHPCS and PHPStan issues?
Also I think this can still end up with some confusion and circular logic.
Thank you so much for this contribution!
I think I'm going to mark 1.x as not recommended and only merge this into 2.x
I'll cut a release later today I think.
nicxvan → changed the visibility of the branch 3426536-adjustment-action to hidden.
We don't need to handle hook_hook_info it will never be oop.
After looking at this and reading #2941155-14: ModuleHandler should not maintain list of installed modules now that ModuleExtensionList exists → I think I agree, I'm not sure we want to change this.
If we do there is also a method getInstalledExtensionNames, that is literally getting the list from module handler.
This seems pretty straightforward and seems helpful also for CMS.
I converted the patch to an MR.
Addressed the phpstan issues in the new extensionconfirm form. Added some items to the remaining tasks.
Still needs work.
I think there might be something missing for uninstalling modules when a theme is enabled, I didn't look close enough. We do need to handle the forms extending the newly deprecated form, and most likely create new ones.
I'll take a look.
nicxvan → changed the visibility of the branch 3100374-make-it-possible to hidden.
While it is a big patch, this is the same process used for core to convert, and we can spot check that the conversions are accurate. Since this is rector it's really just copying the hooks to a hook class an method.
Webform also has more tests than most modules so we can have more confidence than most modules would have that this won't introduce systemic issues.
nicxvan → created an issue. See original summary → .
Neither of those will be fixed automatically, but I am committing to working on both of those follow ups if this gets committed here.
The thing is I can manage and rebase those two fixes in a follow up (or more likely separate followups) in a more standard manner once this is in.
Just to add some context here, this is a bulk rector pass, conflicts for auto generated commits like this need to be just scrapped and redone, they are not like normal conflicts where you can resolve them since it's bulk copying functions to methods. The more manual work on top of that the more fragile it becomes.
I'm willing to commit to resolving these codesniff rules in follow ups, they are generally not complex to fix and once this is in those issues could be rebased with a normal workflow.
The risk of conflict is much lower.
CS is passing now with rules addressing t() \Drupal and one DateFormat calls.
I just noticed we already have an ignore rule for \Drupal calls, it's just not taking effect, let me look at that. If that were in place this would be far more manageable.
I am testing a different tool for addressing the arrays.
As I mentioned it's not going backwards.
These are patterns that exist in the procedural code but get flagged in classes.
If this needs rebase I will, but this still needs review.
I reviewed this, one minor comment.
At first I questioned it being on ThemeManager or a helper, but I think it makes sense internal.
I removed the method from ThemeManagerInterface and just add a method exists and marked it as internal.
Makes sense to me.
For now, I didn't add a new public method to reset the variables, I just added it to resetActiveTheme(). Default variables acually depend on the active theme, and for the use case in user logout/login, it doesn't seem like bad idea to reset the active theme, since the active theme can depend on permission.
I think this also makes sense, but I'm not 100% sure. I do think it's a good thing to get rid of drupal_static_reset, worst case if others disagree we can create a new method to reset only these variables.
I also confirmed the search for template_preprocess were all previous to drupal 8 or distributions that had core copied.
This will definitely cause a conflict for the preprocess issue, but this look great.
I'll wait on the CR and give others a chance to review.
I spent a couple of hours going through, still got a lot to go.
I'd like confirmation on whether we can skip the t() and injections.
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.
Performance test was fixed in 🐛 Performance tests need to run cron Active
The issue is twofold.
t() in .module doesn't throw phpcs errors but it does in classes.
Drupal service calls are the same.
Rector doesn't address these so they have to be manually addressed.
I need to still review the other cs failures.
Would you be willing to having a temporary skip rule for those two?
Sounds great! The only failures I see other than code sniffing are deprecations that are on HEAD so I assume those can be ignored.
I can look at all of the code sniff failures besides t() and dependency injection.
I can create a manual patch for those I think this week.
Ok number 4 has been addressed.
Looks good to me, all performance tests have been updated and this should make the queries like up in tests.
I am getting navigation performance test failures in this, I'm fairly sure this is unrelated:
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.
Runtime: PHP 8.3.10
Configuration: /var/www/html/phpunit.xml
F 1 / 1 (100%)
Time: 00:23.234, Memory: 4.00 MB
There was 1 failure:
1) Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest::testLogin
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
0 => 'SELECT "session" FROM "sessions" WHERE "sid" = "SESSION_ID" LIMIT 0, 1',
1 => 'SELECT * FROM "users_field_data" "u" WHERE "u"."uid" = "2" AND "u"."default_langcode" = 1',
2 => 'SELECT "roles_target_id" FROM "user__roles" WHERE "entity_id" = "2"',
- 3 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"',
+ 3 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.maintenance_mode" ) AND "collection" = "state"',
+ 4 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "routing.non_admin_routes" ) AND "collection" = "state"',
+ 5 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"',
+ 6 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "twig_extension_hash_prefix" ) AND "collection" = "state"',
+ 7 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.private_key" ) AND "collection" = "state"',
+ 8 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "asset.css_js_query_string" ) AND "collection" = "state"',
+ 9 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "drupal.test_wait_terminate" ) AND "collection" = "state"',
+ 10 => 'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "system.cron_last" ) AND "collection" = "state"',
+ 11 => 'INSERT INTO "semaphore" ("name", "value", "expire") VALUES ("state:Drupal\Core\Cache\CacheCollector", "LOCK_ID", "EXPIRE")',
+ 12 => 'DELETE FROM "semaphore" WHERE ("name" = "state:Drupal\Core\Cache\CacheCollector") AND ("value" = "LOCK_ID")',
]
/var/www/html/core/modules/navigation/tests/src/FunctionalJavascript/PerformanceTest.php:73
FAILURES!
Tests: 1, Assertions: 4, Failures: 1.
Found a way forward from @berdir with modulehandler loadInclude.
I am not going to work on this, I think @berdir found a clean workaround, I think my comments about constants.php are still valid.
Yes, I wouldn't create the deprecation here, just create the new landing place for constants and the new constants we need.
I wonder if there is a way to have both.
E,g. something like
/Drupal/Core/Constants
That is where the constants live, you can use it if you need it.
Maybe it makes sense to auto load it so they are available everywhere that uses autoloading.
I am going to work on this because we need these constants for hook_update_requirements and hook_runtime_requirements.
The one downside is this approach is only available to core and not core modules/themes, or contrib or custom because they don't using composer to set up their autoloading. But I'm not sure that outweighs the advantages listed.
Since these classes are used in core, contrib, and custom, I think this disqualifies the constants.php approach.
However, we don't actually have to deprecate these here, I think we can do that when we deprecate hook_requirements, for now we can just create the new classes and just use them as we convert hooks.
We need the constants!
I think everything has been addressed. It's been moved instead of wrapping so we can add return types.
Look great!
We don't need to worry about hook_hook_info, it cannot be oop anyways.
My point was we shouldn't make assumptions about contrib and custom code.
If you think the ignore rule is enough then it's ok with me, this covers the vast majority of the cases.
Migrate tests are failing due to update I think.
Change record for something like this is probably still useful, I haven't rebutted the code yet, but this should is a great clean up.
ThemeCommonElements uses it too, but that should be able to be cleaned up too.
I'm working on this.
rectorv4 is the MR to review just waiting on tests. I updated the script to handle the unused use statements too.
There is literally one implementation in core that only interacts with update, we should just convert all requirements unless they interact with install.
I think this is no longer postponed, but we should only create three children for now.
1. Convert all hook_requirements that only interact with phase update
2. Convert all hook_requirements that only interact with phase runtime
3. Convert hook_requirements that interact with both runtime and update if any.
Discuss here any that interact with install in any way. Discuss whether we convert those phases or wait on 📌 Create class to replace hook_install_requirements Active
Thanks for merging those!
I think we can leave this Fixed even with phase 2 and 3.
I actually think it makes more sense to move install discussion to
📌
Create class to replace hook_install_requirements
Active
Then we can convert
📌
[pp-3] Convert hook_requirements to new classes or hook_runtime_requirements
Active
to a meta for tracking what changes we're going to make to existing requirements.
Just to add to @berdir's great summary.
I think a) and b) are enough to justify this change.
I think there is also c) it significantly reduces the times we need to lap around looking for module_preprocess functions because we now do it once in build, whereas before we checked on every module and every prefix. This is a great change.
1. We do add the template pseudo module, but that is one line in HookCollectorPass and one line in ModuleHandler. As @catch noted, this would likely break currently if someone named their module template, we also significantly improve the DX here by adding #[TemplatePreprocess]`
2. The theme registry is larger, but 17% isn't too bad especially considering it is only a lookup for invoke and as @berdir notes, there are already issues that will reduce that further. The bc check for custom callbacks added theme_registry_alter is unfortunate, but there is an explicit test in core for this, and as @alexpott points out contrib does use this.
3. As @berdir notes, I fundamentally disagree that using invoke like this is a problem, it can be cleaned up when we add the ability for themes to be OOP, invoke was doing far weirder things before 11.1 so this isn't too far astray.
4. That still works, it uses the prefixes not the actual functions unless I am missing something.
This looks great, for baseline I run the following command to auto generate:
./vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/.phpstan-baseline.php
To be honest I'm not sure why you didn't have to add expect deprecation to the test marked with @legacy, I think class deprecations are different so I don't think you need to change anything.
I did compare the deprecation process docs for classes and confirmed you matched the documentation.
The only other thing I'm not sure of is the new unused parameter errors. I think since these are new classes now the preference is to ignore a single line. I'll ask for clarification on this in #core-development. Other than that looks great to me.
Berdir should get credit too, he found it.
I addressed your comments.
Yep I'll take care of it
This happened to me once yesterday too hitting edit.
nicxvan → created an issue. See original summary → .
Unfortunately I think this needs to be addressed in core, @berdir mentioned this is minimally maintained and this is not a security issue. I've been meaning to find the corresponding core issue but I haven't had the time yet.
This may have potentially broken navigation performance tests.
After rebasing
📌
Create hook_runtime_requirements
Active
I am getting failures for the navigation js performance test.
$this->assertLessThan(90000, $performance_data->getStylesheetBytes());
I think the failure is from upstream, nothing here would make the navigation stylesheet larger.
nicxvan → changed the visibility of the branch 3490841-create-hookruntimerequirements to hidden.
Yeah that will probably happen for each module or profile file that disappears.
On new or existing sites?
Steps I took for rectorv3
Checkout that branch in an 11.x version of drupal in ddev
Add this script https://gitlab.com/-/snippets/4769802 to ddev and run it
yes to both questions
Commit rector changes
Find and replace // phpcs:ignore with something easily found (since it breaks codefix) I used // elephant
Run codefix in ddev
./vendor/bin/phpcbf \
--standard="Drupal,DrupalPractice" -n \
--extensions="php,module,inc,install,test,profile,theme" \
web/modules/contrib/webform
Find and replace // elephant for // phpcs:ignore to swap it back
Commit and push
3 known codesniff issues
1. unused use statements, rector expands arguments - not sure why codefix isn't fixing these
2. t() in classes
3. \Drupal in classes, rector doesn't do injection