I agree that this looks good based on earlier reviews I did.
My only uncertainty is in which order we want to proceed. We already completely break BC on modules *calling* requirements hooks. This is somewhat rare but it happens, I of course have a module that does that ( https://www.drupal.org/project/monitoring → , see also https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22modul..., without moduleHandler it finds a lot more, but I assume that's all D7 code).
That's OK I think, this isn't meant to be an API, and I do plan to update monitoring.
But as I mentioned in 📌 Add value objects to represent the return of hook_requirements Needs work , the new hooks gives us a chance to considerably simplify the BC layer that the new value object for requirements definitions needs to support. we could say that the new hooks must return value objects. The it might make sense to do the other issue issue first. But that isn't close yet and there are more BC things to consider (for example usage of #type status_report, which is also used in a few projects, including again one of mine: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22%27%2...).
Long story short, it's probably OK to get this in now and first. Might want to think again about it before we do system module though.
Created a merge request with a proof of concept, basic right now, but already covers BC by only looking for and registering template_ prefixed functions if initial preprocess is not defined.
MR is based on the referenced issue, only relevant changes is the currently last commit: https://git.drupalcode.org/project/drupal/-/commit/a955017c8e669d28297af....
Array key "initial preprocess" obviously up for discussion, I picked this to explain the difference to regular preprocess functions.
I copy pasted the invocation, we can for example use\Drupal\Core\Utility\CallableResolver to also support services.
Tests are failing, I'm afraid you can't leave :)
(Yes, random test fails are pretty bad right now)
Yes, theme config is a bit of an edge case, but doesn't change the fact that third party settings never were supported, it just wasn't validated strictly enough. You can store it in your own config, keyed by the theme or a config file per theme, both is possible.
The example I referenced to is a form alter, not a separate form, nothing has to change in the UI for users.
> Is it worth it to include the contrib minor version?
IMHO, yes absolutely. Yes, in most cases, it won't be needed and old minors won't get updates. But if you ever have that situation, for example due to a security issue that's also fixed on an old minor and you have a continuous list of update function numbers, it's going to be really complicated to untangle. You'd need to reshuffle update functions since some people will have already run a certain update then on the old minor, others will update directly to the latest minor.
third party settings on simple config was never supported. Automated tests have defaulted to validating config schema for years and would have failed on this.
Per #4, you can alter an existing form and store your config in your own simple config, see for example automated_cron in core: automated_cron_form_system_cron_settings_alter()/automated_cron_settings_submit().
bundles have for example bundle classes now. It' not used that much, but there are specific other issues to for example also handle plural labels.
The proposal her is not feasible.
the proposed solution in the iS does need an update, but #72 explains why it's not called. uri_callback is a weird leftover from before we had link templates.
Entirely deprecating that concept is done in 📌 Deprecate uri_callback in routes for entities Needs work , this is just one specific dead configuration that is not used.
Yes, that's exactly the idea. My plan is to filter and only assert on redirect queries. Anything else seems way too much pain to keep up with and support various different minor and major core versions.
Also considering to add one to redis, but similar problems there. I could for example assert that there are no semaphore queries, but we're already testing that in the existing full web test that simply asserts that no semaphore, queue, cache, .. tables exist at the end.
Does it make sense to postpone this on the title issue? If I understand this correctly, that one introduces an API method to fetch the current entity for which to display the title and the same logic could be applied as to when the edit button should be displayed. I think it makes sense if those two things are in sync. We should only have an Edit button if there is a title that displays for what that button?
Reviewed.
That's OK with me, but this has no merge request, so tests didn't run. Please ensure there is a merge request with the latest and tested version of the patches here.
Marking a fixed then, thanks for reporting back.
Yes, set() is a good idea, much better than just invalidate.
This also needs tests, our existing tests pass, but they aren't really good examples, mostly just use URL's that aren't really a path. An actual performance test that lets us inspect if there were redirect queries would be neat.
This will need some adjustments for the recent changes that went in 8.x-1.9 (which is being created as I'm writing this), specifically the last write timestamp optimization: 📌 Optimize bin cache tags and last write timetamp Active .
> If we can get this in without having to reroll again it would make my year 😅
Sorry, as we've discussed a while ago, I'm the sole maintainer of this and I have no experience with redis cluster, so it's hard for me to commit to maintaining that.
For the reports page, there are two open issues about problems with the reports page. It would be _great_ if someone would extract the approach from this, which I really like, ensure that the two other issues are also covered (minus the small cluster specific parts). Then we can get that in, which will greatly reduce the conflict potential this has.
That would also help us move into the direction of an idea for a 2.x release of this project that I have, that would introduce an adapter that sits between cache/lock/queue/tag backends and the actual libraries and abstracts the relevant bits of the different implementations into something that each adapter can then customize. I don't know yet if that's feasible but that has the potential to be a major simplification for stuff like this, this info() method could then move into that adapter.
As for the remaining bits of this, this issue has 50 followers and cluster setups implies that there might be budget so I could invest a few hours into setting up a cluster test setup, on CI if possible, and then we could have this covered with tests. Reach out to me if you're interested.
Also, if this is a behavior that is important for your site, feel free to reach out and sponsor a few hours of my time through MD Systems to finish this up with tests and documentation and I'm happy to commit it.
Again, per #21, this is incompatible with test expectations that core has for cache backends and it will never pass tests, don't bother debugging that, that's by design. Core expects expired items to be returned when asked for. This changes it so that it doesn't.
🐛 Allow to remove support for invalidateAll() and treat it as deleteAll() Active is an example of what I'm asking for. Add a setting that allows to make this a configurable opt-in behavior, it absolutely makes sense, but I want to have redis by default correspond to core cache backend expecatations as much as possible.
I closed one issue as a duplicate, I think I'd prefer to also close ✨ Default lifetime for permanent items for Drupal 8 Needs review as a duplicate and merge them together.
Closing as duplicate of the other issues.
Added phpcs exclude like the the referenced case.
> Good candidates for reviewers are the maintainers of the mailsystem module I guess.
the maintainer of that module is following this issue, I'll try to review soon.
> Yes, we cannot autoload it so there is no way to find it without knowing the naming convention unless you have a different idea.
My naming convention suggestion is only and always name it "InstallRequirements", nothing dynamic. It's namespaced, it doesn't need to have a prefix. Downside is that there will be a bunch of classes with that name, but the long term plan is removing all/most of them.
quietone → credited berdir → .
That's a bug, even though it's prefixed with cache, other systems use the cache_prefix and so should queue. Add the RedisPrefixTrait trait and use the getPrefix() method it provides.
Merged, also created a change record.
Created a CR: https://www.drupal.org/node/3500806 → , applied the suggestion.
quietone → credited berdir → .
This seems to work quite well, tested quite a bit in combination with a few core performance issues I worked on.
Updated the tests.
I've pulled in the performance test changes from ✨ Introduce a list of "common cache tags" to reduce lookup query amount Active and updated the counts to reflect 11.x, then we have a baseline to be able to see improvements.
Suggestions, how about I pull the performance test parts to assert the behavior into 📌 Aggregate cache operations per bin to allow for more specific asserts Active , then we can document the current baseline and it makes it easier to compare improvements?
I did a merge of 11.x (since previous updates also did a merge), didn't review yet if the tests still pass.
I did have a quick look at the current event based idea, but it currently breaks when using redis. The problem is that redis uses cache tags to handle invalidateAll(), which means that even the initial container cache load does a cache tag check. This is being optimized away in both core (by deprecating invalidateAll()) and optionally just in redis for now, but it's there and the current default behavior.
Ignoring that, the next thing is then that this would trigger an event subscriber on internal page cache. Even with caching and what not, that's a ton of extra complexity at that point that we don't want or need as mentioned above.
So I really think this should be externalized into a separate and very early request subscriber.
Maybe, but as mentioned, it's moving up to access policy with 📌 Review cache bin and cache tags of access policy caching Active and then it will be cheap.
FWIW, I have no idea why that whole check exists. There is no possibility that user roles don't have a storage handler? This check was added all the way back in #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system → in 2013 when AdminNegotiator was introduced, there was no similar thing before that in the code that it replaced.
Re #11: Strictly speaking, the tests themself are reported as skipped, not passing, but we allow a test job to complete in case of skipped tests. My guess was that test only is maybe missing the chrome service, but that's there and the tests in #10 are shown as one fail and the others passing.
Try adjusting .gitlab-ci/scripts/test-only.sh in your MR to add --fail-on-skipped to the phpunit command on line 27, maybe then phpunit will be kind of enough to tell us why they are skipped? And/or a --debug.
There is definitely something very funky going on with those performance test assertions on CI, similar to the language caching issue.
I accidentally did this on top of 📌 Review cache bin and cache tags of access policy caching Active . The counts are definitely, 100%, wrong. But the test didn't fail?
Also need to rerun the other tests, they are also wrong.
> In that case would we keep the separate bin for when it's used?
Good question, I'm not sure. Really depends on the implementations of the used access policies. I imagine that with group or so, there could be a fairly large amount of variations, but they should be quite small?
Sites that want to optimize this can configure the access_policy bin use ChainedFast, it's still a separate bin though. Updating it to use a different/existing bin is a little bit weird, since the service name and properties are all directly tied to that name.
Created 📌 Aggregate cache operations per bin to allow for more specific asserts Active for the cache operations by bin stuff.
Crosspost on the RTBC, sorry.
> Cache gets are probably the automated_cron configuration.
Created 📌 Aggregate cache operations per bin to allow for more specific asserts Active so we have the ability to see this a bit more reliably and can be more specific with assertions when we want to.
Pushed a proposal, right now PerformanceData is a pure value object, I added a tiny bit of logic to sum up the counts, I also kept the existing set method calls and aggregations for BC.
I also updated a handful of tests to show this including an additional assert on the cache gets on default. The second non-chained-fast bin with several get operations (7) is data, but I think we don't want to explicitly assert that, no idea how stable those hashes are:
0 => 'route:[language]=en:[query_parameters]=:/en',
1 => 'css:umami:umami:enhDWZUCRilCXGGt6daBK7eVBZ-TLhK-AMv4cB0y_qG7c1',
2 => 'js:umami:en:u_rNnKlC9ds79tQ_PVLa3cfvZ4SZfSwy9XHF-AfxWqA11',
3 => 'route_provider.route_load:e12969cb977b5b9f14e8b365ac441ec725a833d6586e87d7bcfa7b2c5fc214e2e814196e86f1e6d007e3e8b5703635782521189223a9fa225bcebdbdaf38d833',
4 => 'js:umami:en:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11',
5 => 'js:umami:en:u_rNnKlC9ds79tQ_PVLa3cfvZ4SZfSwy9XHF-AfxWqA11',
6 => 'route_provider.route_load:61d3f1526762a7ca8ed3aa9d059d2b475e3fb6a5c3f584efd6daaf7d5b942ab3268da173e50ca03f4166dbbb9e821bebb435727ee84eec58c3d67dc1811b5a39',
Performance tests are very unreliable for me locally. script size is like 4kb off and everything goes completely haywire if I don't add the explicit cron run in setUp(). Is my system too fast? too slow? is it WSL? different chrome version? Honestly no idea.
I was also confused why don't see a change on cache get requests with this on any performance tests except one single cache tag is valid count being reduced in a single test.
The thing is that this trades regular cache requests with fast chained requests, but for Performance tests, those are the same thing. I don't think they should be. Obviously a separate issue, but what about grouping the cache operations per bin? We can keep the current methods and then aggregate, but we can also add methods to assert counts on specific bins and add those to select tests?
Created a merge request, works as expected, combined with the improvements in core (such as the theme extension list cache) and redis (the already committed last write timestamp and the invalidateAll() optimization), this is what's left now up until dynamic page cache
"HGETALL" "core:container:service_container:prod:11.2-dev::Linux:a:4:{i:0;s:4
:42:\"modules/contrib/redis/example.services.yml\";i:3;s:40:\"modules/contrib
"GET" "core:container:_redis_last_delete_all"
"GET" "core:last_write_timestamp_cache_config"
"GET" "core:last_write_timestamp_cache_discovery"
"GET" "core:cachetags:entity_types"
"HGETALL" "core:data:route:[language]=en:[query_parameters]=:/"
"GET" "core:cachetags:route_match"
"GET" "core:data:_redis_last_delete_all"
"GET" "core:last_write_timestamp_cache_bootstrap"
"HGETALL" "core:dynamic_page_cache:response:[request_format]=html:[route]=vie
"GET" "core:dynamic_page_cache:_redis_last_delete_all"
"HGETALL" "core:dynamic_page_cache:response:[cookies:big_pipe_nojs]=:[languag
(Thee's quite a bit more after, such as placeholdered blocks, library and assets, routes being loaded).
This is getting pretty close to what's feasible. The only additional thing I can think of right now is the cache tag preloader, I'm seeing the following standalone and stable cache tags:
"GET" "core:cachetags:entity_types"
"GET" "core:cachetags:route_match"
"GET" "core:cachetags:library_info"
"GET" "core:cachetags:routes"
"MGET" "core:cachetags:config:shortcut.set.default" "core:cachetags:config:user.role.authenticated" "core:cachetags:config:user.role.administrator" "core:cachetags:access_policies"
4 easy ones, to reduce 4 get requests to a single mget.
The last is shortcut and interesting, that one still contains the access policy cache tags. they are still collected and I guess shortcut adds that as a dependency to it. but there's also the shortcut set itself. for access policies, I still feel like role save should just directly invalidate access_policies, and shortcut could maybe use a single list cache tag. hard to predict if there is a scenario where shortcuts frequently change? Then we could consider to preload that too, but that's about saving a single extra query and likely only in some cases.
There is of course also the huge list from dynamic page cache itself, but as argued there, it's impossible to preload that whole, so not much point in looking at that in this scope.
Had a quick look but that seems like the only feasible quick improvement. Even when there is a dynamic page cache hit, there's a very high chance that there's at least one placeholder to be rendered, either bigpipe or initially, both need both the theme extension list (twig render loads that) and the initialized theme, obviously.
There's definitely stuff to untangle between ThemeManager, ThemeInitialization and ThemeExtensionList, all do a bit of loading, and it seems sensible that we'd have an ability to only load and cache enabled themes. But there are far fewer themes on a site in most cases compared to modules, so that's all a problem for another day.
Changing the cache bin is enough to remove the extra cache get on default bin:
"HGETALL" "core:dynamic_page_cache:response:[request_format]=html:[route]=view.fro....
"GET" "core:cachetags:x-redis-bin:dynamic_page_cache"
"GET" "core:dynamic_page_cache:_redis_last_delete_all"
"GET" "core:cachetags:entity_types"
"HGETALL" "core:dynamic_page_cache:response:[cookies:big_pipe_nojs]=:[languages:la....
(The entity_types cache tag is also through theme negotiation, because AdminNegotiator checks user roles storage (?), seems strange, but the best we can hope for is shift that around. And if we stop persistent caching for access policies in core then it would move there)
Merged. Next step: 🐛 Allow to remove support for invalidateAll() and treat it as deleteAll() Active
I tend to start off without to get early feedback. But I'm also aware that helps with explaining the reason, created one now.
📌 Replace hook_requirements with tagged services Active is two thirds done.
If we can get this also into 11.2 then this might considerably simplify the BC layer. New hook use value objects, old hooks use arrays, we just need to merge it together in the places where core calls both the old and the new hooks?
Tested this a bit more, have to clean the returned cache tags so they aren't picked up by the fast chained backend:
Impact of this as a diff of the MONITOR output:
--- compare/monitor_before.txt 2025-01-18 13:48:27.451401512 +0100
+++ compare/monitor_after.txt 2025-01-18 13:49:58.731385550 +0100
@@ -1,12 +1,8 @@
"HGETALL" "core:container:service_container:prod:11.2-dev::Linux:a:4:{i:0;s:40:\"/var/www/html/sites/default/services.yml\";i:1;s:44:\"/var/www/html/sites/develop
"GET" "core:cachetags:x-redis-bin:container"
"GET" "core:container:_redis_last_delete_all"
-"HGETALL" "core:config:last_write_timestamp_cache_config"
-"GET" "core:cachetags:x-redis-bin:config"
-"GET" "core:config:_redis_last_delete_all"
-"HGETALL" "core:discovery:last_write_timestamp_cache_discovery"
-"GET" "core:cachetags:x-redis-bin:discovery"
-"GET" "core:discovery:_redis_last_delete_all"
+"GET" "core:last_write_timestamp_cache_config"
+"GET" "core:last_write_timestamp_cache_discovery"
"HGETALL" "core:access_policy:access_policies:drupal:[user.is_super_user]=1:[user.roles]=authenticated,administrator"
"GET" "core:cachetags:x-redis-bin:access_policy"
"GET" "core:access_policy:_redis_last_delete_all"
@@ -15,9 +11,7 @@
"HGETALL" "core:data:route:[language]=en:[query_parameters]=:/"
"MGET" "core:cachetags:route_match" "core:cachetags:x-redis-bin:data"
"GET" "core:data:_redis_last_delete_all"
-"HGETALL" "core:bootstrap:last_write_timestamp_cache_bootstrap"
-"GET" "core:cachetags:x-redis-bin:bootstrap"
-"GET" "core:bootstrap:_redis_last_delete_all"
+"GET" "core:last_write_timestamp_cache_bootstrap"
This saves 3x2 GET commands for the config, discovery and bootstrap cache bins during bootstrap roughly up until dynamic page cache.
Complete output after with this:
"HGETALL" "core:container:service_container:prod:11.2-dev::Linux:a:4:{i:0;s:40:\"/var/www/html/sites/default/services.yml\";i:1;s:44:\"/var/www/html/sites/develop
"GET" "core:cachetags:x-redis-bin:container"
"GET" "core:container:_redis_last_delete_all"
"GET" "core:last_write_timestamp_cache_config"
"GET" "core:last_write_timestamp_cache_discovery"
"HGETALL" "core:access_policy:access_policies:drupal:[user.is_super_user]=1:[user.roles]=authenticated,administrator"
"GET" "core:cachetags:x-redis-bin:access_policy"
"GET" "core:access_policy:_redis_last_delete_all"
"HGETALL" "core:access_policy:access_policies:drupal:[languages:language_interface]=en:[user.is_super_user]=1:[user.roles]=authenticated,administrator"
"MGET" "core:cachetags:config:user.role.authenticated" "core:cachetags:config:user.role.administrator" "core:cachetags:access_policies"
"HGETALL" "core:data:route:[language]=en:[query_parameters]=:/"
"MGET" "core:cachetags:route_match" "core:cachetags:x-redis-bin:data"
"GET" "core:data:_redis_last_delete_all"
"GET" "core:last_write_timestamp_cache_bootstrap"
There are still 2 cache tag lookups for the bin-tag that powers invalidateAll(), on container and access_policy. I'll create a separate issue to allow to opt-out of the current invalidateAll() implementation and handle it as a deleteAll().
I also want to investigate the access policy implementation.
I agree that it would be nice if we can get rid of most* install requirements hooks but that requires multiple new features (other modules, extensions, composer dependencies, conflicts), not all of which can be composer (we can't use composer conflict because we can not and don't want to prevent them being present, it's about them being installed or not).
hook_requirements() is the last remaining "regular" hook (there's just preprocess and special install/update stuff then). If we have that and preprocess done we can drop that stop parsing attribute and so on. modules are either fully converted or not, no weird middle ground.
In other words, it would be very nice to find a way to deal with it. I was thinking we could just keep hook_requirements with phase install for now until we see what's left once we improve other things. But I think that's hard to deprecate and detect allowed implementations vs not, we can't introspect what $phase it has and what it does with it.
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?
That's the only part I agree with from #31. Everyhing else I very much don't. As #32 said, what #31 describes is like 4 different Drupalisms that nobody else uses and new developers don't know about. Nobody except Drupal uses a .install file. Nobody else has an api module, api.drupal.org only provides info on core, it requires a Drupal-aware IDE like PHPStorm or knowing what to search for to maybe find the hook_requirements() documentation and understanding the *magically named* hook functions. Now you need to look at src/ (where _all_ code will be soon enough), where there might be an Install folder and then a Requirements class inside. Looking up the interface is standard PHP stuff, documentation on that is standard PHP stuff. To me, that is way better DX, especially for developers that don't know Drupal. And we want and must attract new developers.
That said, if we're not sure yet if this has a future, we could also just rename it to hook_install_requirements(), but then we have to explain that this is very different from the other two split requirements hooks. I'd prefer a class but I don't care too much. What I do care about is being able to forget about as soon as possible.
📌 Handle module preprocess functions Active should make this pretty straight-forward. Once we converted the remaining preprocess functions in .inc files in core like system and views, we should be able to trigger deprecations if this is set.
Starting to bringt this into shape.
Unsure on the BC layer on template_preprocess(). I checked https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22templ... and not a single one of those is for D8+, lots of copied core files and weird stuff.
I removed the method from ThemeManagerInterface and just add a method exists and marked it as internal.
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.
And per #41, that is literally the only use case. There are like 5 implementations of template_preprocess_default_variables_alter() hook in contrib, most notably the mgv project. And not one of them bothers with the reset. I'm not sure that hook should exist at all. There are so few use cases, you can do your own preprocess if you really want, provide twig functions and what not. But separate issue.
I added the @trigger_error() to template_preprocess(), but I'd suggest we just don't bother with drupal_static_reset()? I also didn't do a change record yet, same story. Do we really need to bother with a change record for something that has zero known usages? IMHO that just adds noise.
This should come back green now and is ready for feedback/reviews. Comes with extensive explanations on the MR about the changes. Could likely do with some adjusted inline comments, open for suggestions.
I think this is ready then, as mentioned above, this impacts a massive amount of modules, but the solution is trivial and fully backwards compatible, optionally with new hooks and LegacyHook, so you don't need to require 11.2 for this.
@alexpott: there is still phase 2 and 3 to consider, no? (converting all implementations and triggering deprecations)
This is a challenging issue. So far I think I've been the only one to really review this, and my reviews and push-back already resulted in several improvements (IMHO), but there are still open things that I'm personally am not comfortable with. I'm tagging this as needs framework manager review to make some decisions. It's already a long issue with lengthy discussions, so I'm trying to summarize this from my perspective.
One one side, we have good arguments to get this in as soon as possible:
a) This is *the* blocker to getting rid of .module files, preprocess functions are the only thing left that must either be in .module files or .inc files (we also want to deprecate that once core is converted).
b) The awkward situation with the optimization flags that we have to break to be able to optimize the implementation here. Earlier approaches relied on more expensive loops and function-with-legacy checks that required new public methods on ModuleHandler. @catch agreed to renaming it. But that means we need to get it in asap so that only few modules already use the current flag, which will stop working (it won't break, we just lose the optimization for them).
However, there are IMHO a considerable number of workarounds/problems as well as performance/architectural issues with the current implementation. @nicxvan and I don't really agree on whether or not they are problems and their severity, so we need input.
1. To deal with the special template_ prefixed preprocess callbacks, this introduces the concept of a special "template" module, template callbacks are converted to hooks registered in the name of that module. It's unlikely but not impossible that this will cause some super weird conflicts, such as a real-world template module that someone has in a custom project.
2. It increases the size of the theme registry and I guess also the time to process it due to how it stores the callback. Current tests place it at around 17% larger for umami. An extra map that maps functions-that-might-no-longer-be-functions to ModuleHandler::invoke() arguments. Right now it does that for all, I think it should do that only when a function actually no longer exists and belongs to a module. One reason for this is that it attempts to provide BC to hook_theme_registry_alter() hooks that mess with preprocess functions. I guess the question is whether or not we actually need to provide BC for that. I think that is very rarely needed, never had that use case myself. unlike regular hooks, what preprocess does isn't actually final and you can usually just undo it in your own preprocess.
3. Related, it conflates modules and theme callbacks, so theme callback are routed through ModuleHandler::invoke() and it's legacy invoke system. This is something that we could just not do, @nicxvan and I just fundamentally disagree on whether or not this is a good idea :) Supporting OOP preprocess (and hooks) in themes is a a later step with several blockers, that much we agree on.
4. There is an additional lookup step where we use all defined functions and preg_match() to find preprocess functions for theme suggestions that have no corresponding template. (e.g. mymodule_preprocess_node__article() when the current theme has no node--article.html.twig template). This does not work yet with OOP preprocess as they are no longer functions. That means we'll tell modules to convert preprocess functions but some of them just aren't going to work. And it's fairly complicated and dynamic (depending on the current theme) which ones are going to work and which not.
There's some more minor things, but I think those are the primary issues that I see.
I have a number of ideas on how to address this, which are more or less complex to resolve. We don't really agree on what's follow-up/blocker. One challenge is the very specific sort order of preprocess functions (template_preprocess, template_preprocess_FOO, hook_preprocess(), hook-preprocess_FOO()).
* Getting rid of template_preprocess (actually surprisingly easy:
📌
Move template_preprocess, _template_preprocess_default_variables into services
Needs work
)
* Register template_preprocess_* callbacks in hook_theme(), either as callbacks (not very pretty, but easy to do) or converting the whole thing to classes with methods)
* at that point, hook_preprocess() at least for modules could be a regular hook that we call after the registered callback.
* For point 4, we do have all discovered hooks already in the event listener, it should be possible to return everything that's preprocess prefixed, we just need to expose it somehow and decide on follow-up or part of the initial version.
Definitely later:
Since we'll need a solution for 4 anyway, we could turn the whole preprocess discovery on its head and *only* do that. Instead of discovering them one-by-one for each template, we loop over all known and already registered preprocess things and add them to each definition as we see fit. Or even go further and support multiple hooks in invokeAllWith(), like we already do with alter(), just call that. Not sure how much slower that would be. When we no longer have to deal with template_ stuff being in the list and support this whole thing also for themes, this becomes feasible.
The feasibility of both the short-term and long-term ideas depends on the question of whether or not the preprocess function list/registry is something we consider an API and provide BC for.
Thoughts? In short, how much technical dept are we willing to accept to unblock .module removal?
Posted a review on wording stuff.
This looks good otherwise and at this point, the actual changes in the MR are trivial. It will affect a large amount of modules though. https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22Imple... finds 1000 tokens.inc files, and https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22Imple... finds 2500 matches.
This also matches 7.x modules, likely a lot of false positives, but still, no way around the fact that a lot of modules will need to adjust for this one. The good thing is that the fix is trivial, anything can just be moved into the .module file and optionally converted to OOP hooks if desired, which resolves the "large hook implementations" concern. And as long as LegacyHook is used, it's fully backwards compatible.
CR looks pretty good to me. I'd explicitly link the Hook CR so people who are interested in that can find it.
This stuff really has rabbit holes inside rabbit holes.
The migrate EntityRevision destination plugin calls setDefaultRevision(FALSE) since 2014, i honestly don't know why. It loads an existing revision or the default revision, neither should have a reason to change whether or not it's currently a default revision, it should just update that revision. Lets see if removing that breaks any tests.
For the menu_ui stuff, the changes that only partially worked inside 📌 MenuTreeStorage shouldn't invalidate cache tags if the menu didn't change Active actually passed then here. The reason for that is again the partial and incorrect fix that was done in #2850022: Duplicating a non-default revision should produce a default revision for a newly created entity → , which allowed the partial and incorrect implementation in #3041326: Remove 'title' and 'description' from MenuSettingsConstraintValidator when used with content moderation by creating a draft of menu link content when a draft of it's parent content is created → . What we did is accept the changed value for isDefaultRevision() but then the return value ignored the current value and returned always TRUE if the entity is *currently* new. That means that it correctly saves the change as the default revision but then in post save, in \Drupal\menu_link_content\Entity\MenuLinkContent::postSave(), it now sees that it's actually supposed to be not a default revision, so it doesn't update the menu tree storage. Meaning, the menu link content entity exists as a regular enabled default revision, but it's just not in the menu tree. Saving it through any other means than the node form on a draft would then however resave it properly and it would become visible.
What I did there now is as suggested above, point those menu links to the /latest page, then they show up but only for users who are allowed to see the latest version. It's either that or not allowing to create new menu links on a draft. And I changed isDefaultRevision() to ignore a new value on a new entity, so that it doesn't change in the middle of saving.
I also removed the first check about new and not default revision, because both old and new implementation of isDefautRevision() do not allow for that to happen.
I see lots of new fails though, so I'll need to investigate where that comes from.
FWIW, I'm perfectly OK if we completely ignore Content Moderation (and Workspaces) here. The implementation is fairly well abstracted and we can easily do a follow up and start with some basic module exists checks to get the content moderation state.
> in the top bar in the case of content moderation could be outright inaccurate.
I think it's not that bad. It's a bit confusing when it's a pending draft, as it would show Unpublished then. But if you use workspaces, it gets more complicated anyway. It can be published in a non-live workspace and what not.
Also might be useful to look at https://www.drupal.org/project/moderation_sidebar → for some inspiration. It displays the current Moderation state (Draft / Published / Archived) usually and if you're on the published page and there is a pending draft, it says "Draft available". published states are green, non-published is red, an available draft is yellow. a pending draft is also red, I suppose that could be a different color.
Converted the 6 year old patch to a merge requests. Rebase was messy (surprise!) and I will definitely still need to do another pass to clean up remaining references to template_preprocess(), update deprecation and drupal_static() stuff, but tests *are* passing, that part was easier than expected, so setting to needs review for now.
> Claro prepends the "Edit" words into the title itself, in italics:
Technically, Drupal/the node module does that. If Gin is different then it explicitly overrides and customizes that. So we'll need to figure out who does what. navigation will eventually become the default/only/recommended solution in core but it won't be mandatory, so it might fall to that module to adjust things to fit with information in the top bar, but that's something to figure out once the design/goal is clear.
> In a followup, refactor ModuleHandler and ThemeHandler so they have a common base (they used to be the same thing after all) and provide OOP capability for hooks by themes. This likely needs
This belongs more in the meta issue than this, but keep in mind that there are good reasons why themes can't and must not be able to implement any hook. Currently, we only invoke the special theme-hooks (like form alter) for the active theme and its parents. And I think we shouldn't change that. And if we don't, then it's important that themes are not able to implement hooks that may be cached without considering the theme. Because it results in a completely unpredictable situation then. Think of a theme that implements hook_entity_type_alter(), the change will either be there or not depending on whether the cache clear happens in the frontend or the backend theme. The ability that themes can implement any hook was removed on purpose and we need to keep it like this.
berdir → changed the visibility of the branch 3495943-preprocess-combined-approach-with-skip to hidden.
This doesn't require any test changes anymore thanks to #3040878: Reset static entity cache on POST requests in tests → and the permission related problem now just results in a logged warning.
How much this really helps is hard to say, this method clearly shows up in blackfire profiles on pages that load and display a lot of translated entities (this includes menu links of which there can be dozens to hundreds), how much that is blown up to the overhead of blackfire is the tricky questions, but I'm confident that it does help.
The CR says it is from 11.2+, but I think the problem explained in #24 deserves an explicit mention that this attribute class can only be used if a module requires 11.2 since LegacyHook doesn't work for this.
This doesn't only introduce the specific Formalter attribute but also the concept of hook attribute classes for dynamic hooks? I'd suggest we add a second CR that focuses on how to define your own Hook attribute classes, specifically when it contains dynamic parts such as form ids or entity types.
> Nominally this looks good, but I'm concerned that this could prevent something from being added that we would want added and that would be almost impossible to detect.
HEAD already attempts to automatically block all functions that contain "preprocess" (being removed in the preprocess issue, but it's still there now) and update functions (but inconsistently), so I don't think this introduces anything new.
IMHO this is safer and more reliable than the attribute to stop processing in the middle of a file, as it only excludes functions that explicitly match patterns that will never be hooks.
Merge request is green and identical to the patch that was RTBC.
Not sure if we want to keep the explicit test coverage with the direct query, as with the issue that this is blocking, saving would result in an exception instead, we're kind of testing that issue, not this.
entity save always throws an exception when there is an error in saving. It however can return FALSE in case of a non-default revision, for which there is an existing issue, closing as duplicate of that: 🐛 Consider not returning FALSE when saving a non-default entity revision Needs work
> I wonder if we can somehow detect if you attempt to save the default revision as not-the-default revision and throw an exception then or something because that is not a valid thing to do.
good thinking, past-me! current-me discovered this again and opened an issue for it: 🐛 Disallow saving the current default revision as a non-default revision Active . This is blocking that now.
This was RTBC and somehow fell off the bandwagon, lets try to finalize it. Still applies, not sure why it was needs work, random fail maybe?
I think the only bit here that's not addressed yet is search, which is in \Drupal\node\Entity\Node::postSave() now, there are also node_comment hooks that update the search index.
Errors like this typically happens when doing an entity query on a non-existing field.
This is mixing terminology, is it now a content type or an entity type?
comment_entity_predelete(), which triggers the error, just does an entity query on entity id and entity type, that should absolutely not fail. It might be possible that your entity storage for comments is somehow messed up.
Is it really worth doing this when we are in the middle of changing all hook_requirements() implementations anyway?
.gitlab-ci.yml changes apart from temporarily enabling PHP_MAX are out of scope. Those performance optimizations are very much on purpose. the phpunit jobs took 30min+ here.
As commented in the other issue, the job output is full of entity_browser and search_api (and maybe other) nullable types errors, which makes it hard to verify that ther's nothing left in paragraphs.
While not a hard blocker, it would be helpful to make sure issues in those projects exist and are referenced.
> If it’s a view, it should show the views's title. But only on the front-end because it needs an edit button (not on the admin People page for example)
This already sounds like a non-trivial special case. views pages are not canonical entity pages, we'd need to hardcode frontend/backend logic. edit button is also a separate issue and depends on local tasks, views pages don't have local task for editing at the moment, so that would also require custom views specific logic there.
My suggestion would be to just focus on regular entity routes here.
I guess one question is whether canonical pages and edit/delete pages should both just show the title or something else. I feel like Edit %title would duplicate the page title, the screenshot in https://git.drupalcode.org/project/drupal/-/merge_requests/10417#note_43... shows that perfectly. Is that really useful? The same can be said about the frontend, but on the frontend, the title is usuallly further away and not always obvious, depending on the design (hero elements, hidden titles, ...
Created a merge requests, doesn't quite fit the actual issue, can also move it somewhere.
Created a MR with a quick fix, but again, I honestly don't understand what the goal here is and I think that could be simplified a lot if it's internal to OpenTelemetryLogs
And disabling the deduplication feature also causes a fatal error, because it calls $this->logger->onTerminate(); but doesn't check if that actually is an OpentelemetryLogEnhancer object.
How about this idea as a compromise?
Require an explicit configuration of the endpoint, if the field is empty then skip tracing/logging/whatever, but ship it with localhost as default configuration?
if ($this->settings->get(OpentelemetryService::SETTING_ENDPOINT)) {
$this->logger = $this->loggerProvider->getLogger(
$this->settings->get(OpentelemetryService::SETTING_SERVICE_NAME) ?? OpentelemetryService::SERVICE_NAME_FALLBACK
);
}
else {
$this->logger = NoopLogger::getInstance();
}
There might be better ways to set that up, but it wasn't obvious to me.
And make the disable checkbox in the UI clearer: "Disable tracing"
Or, make the disable checkbox apply to everything and then check for that instead of the endpoint. But I actually see use cases for having logging enabled but not tracing. Especially since I won't be able to have a local opentelemetry collector to batch data together, I'm still unsure how much data I really want to collect and process.
> is expected, it just warns you that the endpoint is not available.
This is similar to issues I opened, which changed it from an error to a warning, but I think that's still problematic.
I think there really needs to be a complete off switch without having to uninstall the modules, for example for non-production/local development environments, CI and also initial install.
There is the disable checkbox, but currently it's only respected for traces (without explaining that) and not logs.
Drupal modules can include a link to their configuration site which is shown after install now by default in drush, there could be a runtime requirements hook to show an error if the connection fails.
It used to, but the new implementation no longer needs to load.
> What's the reasoning for the partial namespace import for Drush command attributes? Why not import the whole thing, or alias them if there's risk of a name collision?
I'd say because the attribute name themself are very generic (Command, Help, Usage, ...) and CLI gives them context. Technically it works correctly, but it's just against the common usage that drush itself has established, so maybe it just needs an exception for that.
I'm OK with including this as an intermediate fix until the core issue is resolved and I guess it could in theory still happen.
Please provide this as a merge request and include a svg source file so it can be edited.
I'm a bit unsure if the text on there is too small for a logo. See how small for example the pathauto logo on the project page is: https://www.drupal.org/project/pathauto → .
maybe project browser is a bit bigger, but even then, is it really possible to make out those menu links? Possibly it can be simplified a bit?
Thanks, merged.
Fixed a few bugs in the tests, now there's a sensible amount of fails.
don't really understand what Drupal.KernelTests.Core.Entity.RevisionableContentEntityBaseTest is doing, but I don't think what it does is sensible.
\Drupal\KernelTests\Core\Entity\EntityDuplicateTest::testDuplicateNonDefaultRevision() is 🐛 ContentEntityBase::createDuplicate() should reset default revision flag Needs work , forgot about that, that was RTBC but then somehow it got lost once it was set to needs work. Probably a blocker for this.
\Drupal\Tests\content_moderation\Kernel\ContentModerationSyncingTest::testUpdatingPreviousRevisionDuringSync() is tricky, it's resaving an old revision that was default but no longer is. I'll need to double check if we have a way to detect the difference between current default revision and previous default revision. The migrate tests might be similar, not sure yet.
\Drupal\Tests\menu_ui\Functional\MenuUiContentModerationTest::testMenuUiWithPendingRevisions is the fail I expected. I have one idea that we can try, but it will complicate things a bit and it depends on content_moderation specifically. The idea is that we'd create that menu link to the latest route, where access is denied if you don't have access to preview the draft, and change it back. But it complicates things, we always need to look for that route, or at least if it's a non-default-revision node being edited and update the target on publish.
I know there were other issues about this: 📌 Improve consistency in EntityFormInterface::save() implementations Needs work
Forgot about this issue, see m recent comments in 🐛 Remove return value from EntityFormInterface::save Needs work
Created a merge request, this should fail.
Instead of a filesize, it's also possible to set size restrictions and it will automatically resize it. That said, how big the icon is displayed can easily be adjusted by custom sizes, ours are considerably larger than the default.