Thanks for merging.
TLDR: bootstrap (and config + discovery) cache bins use APCu to keep the caches in memory. All other cache bins always do a database/redis/memcache query. Things that are used often (this is used on dynamic page cache hits that need to render placeholders), aren't big and don't have many variations benefit from being put in those cache bins.
I think my feedback from #24 was mostly addressed already. I did a pass of https://www.drupal.org/node/3499495 → , I put the multiple before/after together used php tags for slightly better formatting and used a real form_id. That is easier to read and understand IMHO.
Not sure yet about doing a grouped change record yet. FormAlter is quite special and worth mentioning on its own. So is Preprocess from [#3499788] I currently don't expect we're going to add a dozen others in 11.2. We should IMHO first do an issue to figure out how we want to approach that exactly, if all hooks should get one or or only dynamic/common ones (see also questions in related entity issue). That doesn't block this issue IMHO, no question that it makes sense for FormAlter or Preprocess.
I also did a pass of https://www.drupal.org/node/3499788 → . I found the "pattern" part confusing, it wasn't really clear IMHO if that was referring to the prefix/suffix or the dynamic part inbetween. All Hooks have at least a suffix, so they all "have a pattern". I also removed Alter from that, those examples did not represent what the code now actually looks like.
What has _not_ been addressed IMHO is #39. Alter right now is quite confusing. I suggested that earlier with the idea that we would support inheritance of the constants, which we don't. I find that a bit sad, but as mentioned in #39, I can not comment on the complexity of that so I'll accept that. However, in that case, I propose that we focus only on FormAlter here and do not introduce Alter. Then we can also delete that change record.
Working on getting this ready. Did another full pass including all the changes to module and include files. Still a few things to clean up.
This also is related to 📌 Create a way to declare a plugin as deprecated Needs work . Ideally, a deprecated block shouldn't show up anymore as a possible option in the UI, but you should still be able to view and edit a block that's already placed, possibly with a warning in the UI.
We had custom logic for this for the node type visibility plugin in D8 or 9.
You want to deprecate the block *plugin* and remove the block configuration from standard profile so it is no longer used.
Reviewed. We can not remove the storage handler here, that will break all existing usages.
> Any reason behind the file and function names being inverted?
I assume you mean classname?
It's the same with all the Preprocess classes that I added. Class is FooPreprocess, and then the methods are preprocessFoo. with ThemeProcess it was just so that Foo wasn't the same thing so far.
Makes sense to me, for the class name it's just following naming rules (repeat last part of namespace if otherwise not providing enough context/being unique enough), while methods are verbThing.
Yes, as mentioned in #4, I've now split off 📌 Convert template_preprocess_pager() Needs review and 📌 Convert field template_preprocess functions in theme.inc Needs review now. I did add authorize report, it's trivial with only two uses and no dependencies and it helps 📌 Remove 'includes' support from hook_theme() Postponed
That removes the two most complex cases and about a third of the size off this one.
I did my best to not include any changes except DI and required reflowing of some comments due to the changed length limits due to them being two spaces more nested. Try using git diff 11.x --color-moved=dimmed_zebra --color-moved-ws=ignore-all-space` to review these issues. It should mark all lines that were just moved and not changed otherwise grey. Doesn't work for the docblocks because those are copied and there don't seem to be any options to highlight that.
Did that last part now, can also copy #7 into the meta issue to provide some more instructions on the approach so this an be distributed, dev days might be a good opportunity for people looking for fairly easy core tasks.
These functions are not an API. While not explicitly listed on https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → , they are essentially hook implementations and as such excluded from the BC policy IMHO.
Core just moved thousands of hook implementations without BC in a minor release. We're providing BC for these because it's only about 100 or so, and doesn't came with the same performance concerns as regular hooks.
Some of the more common ones like container, table and block have some direct calls to them because people sometimes do funky stuff with creative reusage/inheritance of templates and their preprocess logic, but it's pretty rare.
Note that there is not a single known direct call to any of the functions being deprecated in this issue (http://codcontrib.hank.vps-private.net/search?text=template_preprocess_s...), only a handful of @see references from copy & pasted templates. Nothing changes for those templates, nothing changes for people using #theme => 'xy' in render arrays.
This is very different from plugin annotation/attributes for example, where each plugin type needed be converted on its own before you could convert them, so it was important to list which was converted when and they got their own CR or at least a grouped one per minor.
I just think it's easier to apply the same blanket deprecation message to all of them instead of discussing for every single one whether or not we want to do it.
I also just added this to the linked CR: https://www.drupal.org/node/3504125/revisions/view/13953364/13959196 →
(keeping this at needs work because there's one more in a test module that I want to update anyway)
This is just following the existing deprecation. See the meta issue. There are 100+ of these functions, we do not need a CR for all of them.
That setting is only for interface translation, if your config language is not en you need to load english overrides. But maybe we can check the lang code instead?
Merged.
There is no magic class name here. It's just on conventions on how hook classes are named
Still postponed, but wanted to have a try, all we have to do now is trigger a deprecation if one of the two keys is set, started a MR based on 📌 [pp-1] Convert Template Preprocess hooks to OOP equivalent Postponed: needs info which removes those in system module and a quickfix for authorize report that I might move to 📌 Convert Convert Template Preprocess hooks in core/includes Active , there shouldn't be too much left after that.
Started with the ones directly in the module, there are also some in test modules within system module.
template_preprocess_layout_test_2col() is tricky, because it's an indirectly defined theme hook through a library definition. We can't add an initial template on that. The question is, do we need to retain that capability, or could we just make this a regular preprocess hook? It would still be possible through a registry alter if you really need to be first. Alternatively, we would need to extend layout definitions.
This now moves everything in theme.inc, but it's a lot: "115 files + 1291 − 775" (the template references add a lot of files, and the BC layer duplicated documention results in a lot of additional lines being added but that's mostly temporary)
I kept pager in a separate commit so far (has its own pretty large unit test that requires a lot of changes and it's a lot of code) and field preprocess as well (also a lot of code, references from other preprocess functions and a internal but used sort callback, see MR review).
My proposal would be to split those two up into separate issues. They will all conflict due to uses in theme.inc and ThemeCommonElements, but not much more I think.
There's also a question about internal vs final, especially those generic template preprocess functions like table and container are fairly frequently used in contrib. For me, being able to have them final and not worry about constructor BC is much more helpful then the public API of them, about the only thing that could happen is that we deprecate a template I think, then we could just deprecate it and let it be there.
Might need to split this up into more chunks. With deprecations, reflowing of the code and all those references in templates, it's a lot of changes, already at 530 insertions(+), 314 deletions(-) and only did the part of theme.inc that I put in global ThemePreprocess, there's still field templates, pager, breadcrumb, image, menu and more.
Rebased this, had to adjust script size. The diff is a bit misleading because other changes were already causing it to be very close to the upper limit of the 500 +/- range, the actual value before was 170941, now it is 171197 (which I rounded up to 171200), so a difference of 256.
About 110, some already converted now.
I'd prefer splitting this a bit, thoughts:
* One for remaining bits in core/includes? (32, including already convered ones)
* one for views + views_ui (about 25)
* one for system (13)
* one for the remaining ones ? (110 - 32 - 25 -13 = 40)
last chunk still seems pretty big, so maybe split out those that have 4 or more on their own (update, image, file). they all also have include files that can then be removed completely.
For modules, what I'd like to do is move their hook_theme() + the preprocess callbacks into a new ThemeHooks or ${Module}ThemeHooks, that does make it a bit bigger though, but otherwise we have to move it again.
I thought about this, but I'm not quite sure if this is really the right thing to do.
This would pull all these entities out of dynamic page cache as well, is that really what we want to do? Maybe, but also seems to have fairly big implications that we should carefully think through.
should we also exclude them from bigpipe by default?
for views in blocks, this will result in placeholders within placeholders?
what about entity references, comments?
some entities might be not be cacheable, that could be costly. People do use views to render paragraphs for example more often than I'd like.
I was thinking to keep the description a bit more open, like "Control the visibility of those fields on the manage form display page". Also not linked, because you don't want people to jump away without saving, it also won't work yet on a new node type where it's most useful.
But anyway that's two -1 against my +1, so consider me overruled and lets revert this.
The query and cache count on those assertions is sometimes very large, so I expect that's going to change quite a bit in some of the ongoing performance related issues.
Might also not hurt to use https://git.drupalcode.org/project/drupal/-/jobs/4929825 to run those tests 100x to check if there are random fails, I'm not allowed to do that though. There were a bunch of random fails in recent pipelines, but none on those two tests.
In some ways, I'd like to see (and assert) for example those queries on a cold cache, but it's just too much right now. We can possibly revisit this after things stabilize and some performance issues such as 📌 ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load() Needs work are in. but even then, it's a lot of entities that we're loading on a cold cache.
One minor remark.
I moved date/time related preprocess to a separate class in the Datetime componen, that only needs the DateFormatter injected. Then we also have an example that we can build on for example for field templates. All other dependencies are for page and html templates, I don't think there's benefit in splitting that up.
FWIW, re #25, I'd suggest to not extend the core controller and define your own controller and route. sounds like you override and customize more than the controller actually has code for in the first place, so I'd duplicate into a custom route. That should also make the whole thing much more future proof. See also 📌 Deprecate NodePreviewController::title Active . Per https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → , I think we wouldn't even need to provide BC for that.
on topic: Same page preview is interesting, but I don't think there is a good reason for it to be in core, considering the product strategy changes given with Drupal CMS/XB and so on where the focus of core is on providing APIs and building blocks that contrib/CMS can build stuff on.
Re #45. I see the concern about UX review, but unlike the umami suggestion, this change actively affects the experience for site builders and might be confusing. so I think it would be good to find a decent wording for that and do it here.
Makes sense to me. Fine given the change record already exists, for *me*, this one falls in a category of just the issue being sufficient, similar to constructor changes. There is a very good chance this impacts literally 0 sites.
Yes, but the Welcome! is a "no results override", that's exactly the problem. The view has no default title.
I originally came here to close this as outdated or something since it hasn't been updated in 10 years.
I was however a bit surprised to see that while we do now have some intro text on the default frontpage on standard without any content, the problem is essentially unchanged on D11 standard with olivero once you have an article.
Moving to node for now, because it's essentially about the default frontpage view provided by the node module. I think fundamentally, this is a fairly basic accessibility problem. The default frontpage in Standard install profile has no h1/page title, which is not correct and should be addressed. If the view would have a
above block or so that says something like "Recent content" or something, this would IMHO sufficiently solve this. The hard part is probably finding a useful title. The page title is just "Home". For simplicy, we could start there so that they are in sync?
That's fair, created 📌 Hide promote/sticky fields on node types that do not use them Active and also pinged in #drupal-cms-development about reviewing the visibility and usage of those two fields in Drupal CMS recipes.
One thing I just realized is whether we want to update the description of the promote and sticky default value settings on the node type forms, currently it says "Users with sufficient access rights will be able to override these options.", I think it makes sense to extend that a bit and include something like "if they are enabled on the form display settings" or so, either as part of that sentence or an additional sentence. maybe also mention that some are not visible by default somehow.
I think that is a pretty good place to tell especially new site builders who click through those tabs to learn what's there that they might need to think about whether or not they should make those fields visible?
What #49 said (and what I said in #41). This shouldn't happen, only thing I can think if is doing things directly in the database and bypassing entity API. (#49 is not quite correcting that regard. You don't need to set it to NULL, that's not what the scenario is about. It's about either setting it to a non-existing UID or deleting said user record without triggering entity hooks).
But I'm not against hardening our code against invalid data. I probably wouldn't even have asked for test coverage but great that you added it anyway.
Won't change this back from RTBC, but I do one to reiterate one more time that in my opinion per comment #121 and following, the issue summary and change record does not make it clear enough that the second pattern (checking the current route) is not a deprecation, it's a hard BC break. It's different, it will break code doing that and there is no feasible path to avoid that. At best we can build on our tooling to support with fixing the code.
If the decision is that this change is worth it then so be it, but we should IMHO be more clear about that in the change record and the issue summary (which doesn't mention that at all), and maybe it's worth doing a release notes snippet for this. And the change record should also be updated to use a pattern that is backwards compatible so your code doesn't have to depend on 11.2 (change to an is_array() with both routes).
Tried to investigate the failing test. There's an issue with bubbling of that stuff. Within VariationCache::set(), that cache context is correctly optimized away and the cache tag added, but the information doesn't bubble up because RenderCache::set() doesn't do anything with the cacheability object. and I think it should. But it seems to work in the other test method, just not the node within the test entity.
the constants are passed to the mark template. They are defined by it and that's why they are in theme.inc. both node and history module are just using them, not owning them.
That makes BC really challenging with an enum and moving it, we'd need completely deprecate and replace the theme hook, but that would break anyone using it.
Looks good. In a way, it would be nice if we could update standard page to have those hidden, but the default frontpage view doesn't limit by node type, so a promoted page would still show up.
Maybe we can update umami and remove it from those where it has no effect? But that could also be a follow-up, kind of easier to show BC behavior?
They key word here is "by default". The only thing this setting does is whether or not the new revision checkbox is enabled by default.
I agree that there are use case to not see them, but that would be a new feature, there might be issues for that, or you can do a bit of form alter.
kristen pol → credited berdir → .
Worked a bit on this to clean up the unit tests so we can see the other tests now that they are blocking. I adjusted some and remove several that were testing the interaction with the cached preload routes.
Two performance tests then failed, one with the is root exception, the other changed how the query looked but otherwise pretty minimal impact. It did drop one query though, not exactly sure what happened to that.
Missed that we we need [#3476421: Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageCoolCache()] for the scenario where this matters.
Note that 📌 Add database and cache assertions to OpenTelemetryFrontPagePerformanceTest and OpenTelemetryNodePagePerformanceTest Active is kind of a duplicate of that one, should clean up those issues a bit to avoid duplicates.
The combination of those 4 issues means we now have efficient get multiple cache loading for placeholdered routes and we have quite a few of those now with menus, block_content, local tasks and so on being placeholdered:
📌
Render the navigation toolbar in a placeholder
Active
✨
Optimize placeholder retrieval from cache
Active
✨
Optimize redirect chain retrieval in VariationCache
Active
📌
Create placeholders for more things
Active
So I think the benefits for blocks aren't that huge, you'd need multiple non-placeholdered blocks in the same region for this to be efficient. We might want to instead look into either placeholder even more blocks or then not cache them on their own at all # 📌 Add CacheOptionalInterface to more blocks Needs review ).
Looking at umami, the only blocks that are not placeholdered are: umami_search (simple form, pretty fast to build), umami_branding (also pretty simple), umami_messages (just puts in a placeholder, definitely could be non-cacheable?) and umami_help (unsure, but depends on route, so high amount of variations). So, not a lot of benefit here.
But I think one scenario where this is still interesting is what I mentioned #6. If you have a view with 50 nodes that gets invalidated, then you can fetch them with a single cache get instead of 50 (with cache redirects, possibly even 2 instead of 100). To measure the benefit, we'd need to create a handful of articles for the frontpage view.
We'd need to run the children through getMultiple() and somehow flag those that have been fetched, so we don't attempt to load them again.
We just had a client report the same on a site that was updated to 10.4.
Found a few contrib projects that reported this error, such as 🐛 align, frameborder, longdesc, scrolling are deprecated. Active , that helped me track this down to a custom ckeditor plugin in that project, so it's an API change in ckeditor, but fix was easy. Couldn't reproduce on our distribution.
The backtrace doesn't show where table.js is located, but it's likely a patched or custom file. Search for "new Model" and change that plus imports to new ViewModel as in that contrib commit.
yes, we IMHO need to restart 🐛 Views handler loading should respect configuration Active , the approach I suggested would give us a way to respect the config but in a constrained way (use the plugin in config, but only if it is in a list of supported plugins. Then we can add numeric and entity reference here and existing sites will continue with what they have and new sites will default to entity reference. and then issues to pick the plugin based on the list of allowed types in the UI as well as being able to deprecate some.
Yes, also not very fond of what Trash module does.
Just checking seems like a sensible first step. Note that TMGMT will still incorrectly give users he option to chose the publication state as long as it implements that interface, which might be a bit confusing.
Instead of dynamically generating, you could possibly have a few predefined combinations of classes to use, but that can obviously quickly result in a considerable amount of permutations of having a status/owner/created/...
IMHO, if an entity implements EntityPublishedInterface then it should deal with that and it shouldn't be necessary to check for an entity key. That could in theory break some cases that chose not to set the entity key and have their own isPublished()/setPublished() implementation.
That seems like a bug in your custom entity type/with ECK. If you implement EntityPublishedInterface then isPublished() and setPublished() are expected to work.
Bot seems to be confused, maybe because this targets 10.5.
As mentioned back in 2017 when it was still swiftmailer, core is slowly moving towards including symfony mailer, which supports smtp and many other things. symfony/mailer has in fact been in core for a while and technically it's possible to send SMTP mails with just core now, but there's no UI yet to configure this yet. See https://www.drupal.org/node/3369935 → .
So, closing as a duplicate of that issue.
Yes, 3 fewer is exactly what I'd expect, 2 redirects for the search block and one for branding that we cache statically. I think trying to statically cache the cache hits as well is not worth the complexity. It's an exception that you really render the same cacheable render array multiple times on the page, like when doing that kind of twig customization as umami does.
I opened 🐛 Umami page template renders header regions multiple times Active for that, I also have some vague ideas on how to improve that in general, but that could be very challenging with BC (the problem is that render arrays aren't passed around by reference to twig commands, so we don't benefit from the fact that the renderer stores the built thing in #markup/#children).
The wording of some comments seems a bit unusual ("You can read up..", usually we just use a neutral "see xy" but there are a handful of existing "You can ..." in docs already) and slightly repetitive (the array keys are kind of explained twice for $redirectChainCache). But I'm not a native speaker and this is complex stuff, it doesn't hurt to be a bit verbose I think.
Implementation looks good now to me.
Tested, works fine.
kristen pol → credited berdir → .
I know. I'm not saying that's the problem. The problem is that redirectChainIsValid() incorrectly still assumes that we do store a full chain including hits in $chain and it's validating it under that assumption. As a result, it's silently failing those validations and _not_ using them in case of cache hits.
So either we make it less strict for those cases and not validate the last item if that's a redirect, or we stop pretending that we support that and _only_ store chains that end in a miss. This would have the same result with the current performance tests right now. Usually you'd expect that with the optimizations that we now have in place, once we have a cache hit, we no longer need the chain as we won't need to look it up again. Except the umami tests show that with the current twig approach (which is quite common), we do in fact render the same cacheaable element multiple times on a single page, so being able to at lest keep the redirects is useful in that scenario.
I had a closer look at what exactly those 24 cache gets for the NodePage cache are. The first 10 are rendered nodes and medias, that makes sense.
But then it gets interesting, I get 3x the umami_search block:
[10]=>
string(162) "entity_view:block:umami_search:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
[11]=>
string(190) "entity_view:block:umami_search:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
[12]=>
string(162) "entity_view:block:umami_search:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
[13]=>
string(190) "entity_view:block:umami_search:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
[14]=>
string(162) "entity_view:block:umami_search:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
[15]=>
string(190) "entity_view:block:umami_search:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
And then 2x umami_branding:
[16]=>
string(164) "entity_view:block:umami_branding:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
[17]=>
string(192) "entity_view:block:umami_branding:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
[18]=>
string(164) "entity_view:block:umami_branding:[languages:language_interface]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
[19]=>
string(192) "entity_view:block:umami_branding:[languages:language_interface]=en:[languages:language_url]=en:[theme]=umami:[user.permissions]=77981d129fc903b842d6e469f96b1692c3ac47e0369f7b7876856518ae7131b3"
The reason these multiple calls happen is because of core/profiles/demo_umami/themes/umami/templates/layout/page.html.twig, specifically this block:
{% if page.pre_header|render|trim is not empty or
page.header|render|trim is not empty %}
<header class="layout-header" role="banner">
<div class="container">
{% if page.pre_header|render|trim is not empty %}
{{ page.pre_header }}
{% endif %}
{% if page.header|render|trim is not empty %}
{{ page.header }}
{% endif %}
</div>
</header>
{% endif %}
So pre_header is being rendered a total of 3 times. Not sure why the template bothers at all with that, it's a demo, it's not really meant to be customized. But it could store those things in variables. But that's a separate issue that I'll create.
However, I'm also not seeing the static cache kick in for those redirects, that would save another 2 lookups for umami_search at least for now. The reason for this is that redirectChainIsValid checks that the resulting cache id for the redirect chain is in $chain. But because we don't persist the cache hit in the $chain, this will never be TRUE for any lookups that were a HIT, it only works for misses.
I think that's in scope to be fixed here. Not exactly sure what's the best approach. Maybe we only need to validate entries that have a FALSE at the end of the chain? or specifically skip the last item if it's a a redirect?
local git rebase automatically identified the cache tags preload commit I included as redundant, so this should be fine, lets see.
Yeah I was also wondering this - if it's a 'normal' bug and the fix is self-contained (e.g. doesn't require adding some kind of declarative API) then it doesn't feel like a blocker.
I was wondering about that.
We can have a self-contained improvement to cover more cases, but it's probably not perfect. I've seen some discussions about what and if there should be a primary action on views pages for example, specifically admin pages such as admin/content and admin/people and whether the action there should be add user or edit view. Neither will be covered by this. But add user would be possible through the local action system that we already have.
berdir → changed the visibility of the branch 2232375-make-language-switcher to hidden.
Still postponed, but I've already rebased this on top of 📌 Try to preload cache tags in cache getMultiple Active to confirm my assumption that it makes sense and the answer I think is yes.
It's really cool to see how all these performance issues don't just add their improvements, but they actively combine to result in even nicer improvements.
This reduces the cache tag lookup increase for the dynamic page cache hit that this issue has on top of HEAD (3 to 8) to just 3 to 5. The reason it's multiple is that we have 3 groups of cache tag lookups. One is the menu cache tags being separate from the active trail cache lookup for the cache contexts, then we have a filter format cache tag from RenderCache::getMultiple() because that one apparently doesn't have a cache redirect and then all the other placeholdered blocks together. The reason it's a regression on HEAD is that previously, they would have been part of dynamic page cache, even though we explicitly asked for them to be placeholdered. This might sound like a regression, but it's actually good, because we want them to be separate from dynamic page cache, I think the current behavior also mean that autoplaceholdering is pretty busted because on cache hits on the inner elements, they still bubble up their expensive cacheability metadata to dynamic page cache.
And in multiple other cases with dynamic page cache misses, this saves 3-6 additional cache tag lookup queries in many test scenarios due to them now being multi-loaded.
Attempt at a new title
Created a MR for the interface backport.
> It was also concluded some type of access checking cannot be implemented as a query level access check, only as a single entity level access check which means the $entity->access() calls remains a necessity even if they break pagination or may lead to performance issues.
I don't agree with this. Core doesn't do it in hardcoded lists, views doesn't do it. Doing query access is expected to be sufficient. If that _were_ a necessity, then the fact that core doesn't do it would be a security issue.
As quickly discussed on slack, changing how the links are build is challenging because it's a pluggable system depending on your language negotiation settings. See \Drupal\language\Plugin\LanguageNegotiation\LanguageNegotiationUrl::getLanguageSwitchLinks, session/cookie based language adds its own query parameter that it then uses on the given request to put it back into a cookie. It's not impossible but tricky with BC and relying on this happening and so on.
FWIW, I'm reasonably certain that building this is faster than fetching from cache, based on renderering time debug output (had to hack it to show times also for non-cacheable elements), it's also the same right now in HEAD. But there might indeed be edge case such as it being the only thing it has to render with twig.
I also verified that this fixes OpenTelemetryFrontPagePerformanceTest.php with 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed , which currently fails as this block prevents it from being a page cache hit.
It's also no change compared to HEAD. The block is currently being autoplaceholdered and not cached as well, just also in a way that that information up.
I also created 📌 Add CacheOptionalInterface to more blocks Needs review as a follow and want to add a change record for this.
the route name is just as much convention as the local task, it's probably more reliable than the local task as route names are typically generated while local tasks aren't yet.
Could go a step further and look for __entity_form in the route definition, but that's also an extra lookup.
Either way, doesn't really seem like a stable blocker to me? extending this to support more cases won't require API/structure changes or anything like that?
Issue summary updated
Updated, also shortened the test method description, it was already over 80 characters and that made it even longer.
✨ Use modal in add new field flow Active completely removed the exception. In manual testing, I don't even get far enough because there's an assertion in \Drupal\Core\Field\FieldTypePluginManager::getGroupedDefinitions(), but that was there before and it's not as helpful I think. Maybe instead of that assert(), we should throw the exception there?
Leaving at needs work, need to update the issue summary with the options and questions.
Rebased.
Rebased.
FWIW, I specifically suggested to not introduce a helper service for the node indexing and keep them as protected functions. It's not an API that taxonomy module offers, so it makes more sense to me as protected methods. Yes, the logic is duplicated then on the functions, but I don't think there is a rule against that.
Entity browser and our project tests break completely with this, something here isn't correct.
Looking at the response, it has no cacheability metadata at all. $response->addCacheableDependency(CacheableMetadata::createFromRenderArray($preview)); isn't working, $preview not yet rendered at this point, the cacheability metadata is in $preview['view'] and not picked up by createFromRenderArray().
IMHO, this shouldn't rely on the rendering to set cacheability metadata on there IMHO. The controller implements and uses specific query parameters, such as view_name and view_display_id. It should IMHO always explicitly add those specific query parameters or just url.query_args to the response, otherwise there might be edge cases that might be missing them, for example a weird display that outputs something unexpected?
📌 Add JSON:API performance tests Active is now in, which means we have hard proof of how much even a static cache would benefit and can adjust those performance tests accordingly.
I also tested a repeat test run, but those are missing the chrome container: https://git.drupalcode.org/issue/drupal-3438070/-/jobs/4781741
I removed one extra request.
Locally, the test passed without any frontpage request and no sleep but on CI it didn't. Unsure how relevant it is to warm up the regular front caches and asset stuff at all. I tested once by going to /jsonapi overview instead, but that for example removes the path prefix lookup for /jsonapi and I kind of like having that once in there.
Changing the subscriber to be after routing/dynamic page cache exposes the problem again for things that are not part of dynamic page cache.
This already happens on HEAD, but there it still hits the data cache. OpenTelemetryAuthenticatedPerformanceTest exposes this perfectly. Before the last change, I was able to move 2 cache lookups from data to bootstrap because it has the cacheaable list there (This is the isFrontPage() check in LanguageBlock, so very similar to redirect module)
I think this is doing too much at once, but I'm not quite sure yet how to cut it yet. Will try to work on an updated issue summary when I have more time, but essentially, I guess we should keep the data cache for non-optimized cases, then we should mostly be able to keep the status quo. And we can either optimize regular requests and ignore json:api for now, or optimize that. And considering that we now have a lot more blocks being rendered outside of dynamic page cache, I think it makes more sense to focus on that. A separate issue/request subscriber could still add jsonapi routes on jsonapi requests, bonus points for figuring out how to not add the regular ones.
Remaining is also BC (added a new method to an interface for now for the unit test) and naming (RoutePreloader no longer preloads routes) things to figure out.
Merging in the json api performance test, as expected, getting 7 or so extra uncached queries atm.
Testing my idea of storing routes per format, instead of only html. on umami, that results in this route count after enabling rest and jsonapi:
so, 140 HTML, 225 for jsonapi. And that's not a huge amount of bundles and field. jsonapi creates a 2 routes for every bundle plus 2 for every reference field/relationship. Seems like that should be possible to do using parameters, but that's a different topic. I didn't enable any resources for rest.
I'm a bit surprised that I didn't see any ajax/js formats, I guess those routes are using html, so I also assume that currently they are not preloaded as the request format then afaik doesn't use HTML. Maybe we want to unify them to HTML? So many scenarios where performance test could be interesting (views ajax, form ajax?).
Anyway, implemented that, and expected the number of queries to go down on the second cached request then. Except they didn't. The reason is that jsonapi is faking support for the request format. See \Drupal\jsonapi\Routing\EarlyFormatSetter. But with my request event priority change in this issue (to fix route caching for lookups coming early from redirect module), it doesn't yet see that. Reverting the weight change for now gives me the expected result. All the data cache lookups now shift to bootstrap lookups. Whether that's really worth I'm not quite sure.
As far as the entity API is concerned, *create* is the operation that creates a new entity object, not when its' saved. For a form, that happens kind of the first time you access the node add form. There are some edge cases form state cache, so on a form without ajax and without permission to set a created date, you _might_ actually get it to use the time of the submit, haven't tested.
I understand that the form workflow changes a bit how the API works due to storing the value and the created entity, but it's not really a form and even less a node.module issue.
I get that this is in some cases unexpected, but changing this, especially after it has worked like this for 10 years, is also going to run into some edge cases. What about previewing an entity before it's saved, what would you expect there for something that displays the creation date? Also, as soon as drafts and workflows are involved, you're likely more interested in a published time than a created time anyway in many cases? Meaning, changing this would still not really fulfill expectations that people might have?
Having a standard setting for this would allow redis to adopt this as well, by respecting this with the TTL (which redis currently doesn't set by default: 🐛 TTL handling broken, always permanent Needs work )
I'm going ahead and closing this as a duplicate, see all the issues that were recently worked on in 11.2, specifically the children of 🌱 Optimize render caching Needs work .
Blocks can now force a lazy builder placeholder, and we have the ability to bulk-load them from render cache, we have CacheOptionalInterface and either 📌 Make language switcher block cacheable Postponed or a spin off of that (if it doesn't happen there, we could reopen this) will implement support for respecting that in BlockViewBuilder.
No updates since 2016, so it looks ike at this point, nobody really considered this a problem?
Created a second MR that uses a forced placeholder and the CacheOptionalInterface. This makes it kind of similar to HEAD, no impact on performance tests, but unlike max-age 0, this doesn't bubble up and shouldn't break page caching once that looks at that. It could maybe also use 📌 Allow the messages block to skip placeholdering Active once that's in, but would require an alter hook I think to set it on the right level.
Trying to go back to keeping it cacheable but using the new forced placeholder ability. That kind of has the same benefits to cache, although that's mostly FastChained lookups anyway. twig debug shows that it's pretty fast, but it can get more complex with multiple languages I suppose.
I could also experiment with CacheOptional + placeholder and maybe also skipping big pipe (
@smustgrave: We're not fixing those issues. This just changed the render context/time so we're not longer applied by it. In that mode, it also can't be reverted, because that was the whole point of this change. That said, the new placeholder approach might again change how this works, we'll see, didn't run that test.
Note: cache_control_override, at least in it's current from, does _not_ deal with page_cache, see
There was lot of activity on 🌱 Optimize render caching Needs work and related issues in the last few weeks and months. That said, I don't think that is actually blocking this at all, as it's just optimizing (the scope of that meta changed a few times).
So removing it and reducing pp, although i have no idea what the correct number for that is. The most obvious one is still the language block, but I'm working on that.
> Things setting their max-age to 0 to declare themselves as uncacheable
> Would love to hear if people agree with the splitting up of this problem and if so, whether or not the first (nonzero) part should happen in this issue and the zero part be handled in a follow-up or if we should keep the zero part here and split out the nonzero part.
In fact, our custom response subscriber does exactly that, it ignores a 0 max-age. But we've also been using the language block patch for years. The problem is that in this scenario, anything setting a max age 0 will remove the ability for anything else to set a finite specific cache max age. Most multilingual sites have a language switcher (or multiple) on every page, so this won't do anything practice for them.
I recommend rebasing this so we can see the current effect this has on the performance tests.