#D8MI

In light on the comments in #32, which I agree with, this should be closed.

So in agreement to close?

I think this is at NW based on latest comments.

Two remarks, but I don't see why this MR can't be RTBC once they are discussed/resolved.

Leaving at NR as there are no actionable items, yet. Depends on where the discussion leads us.

Also, autoplacholdering doesn't disable caching like max-age 0 does, it would still be cached on its own.

Which is why I added:

Then all we'd have to do is adjust RenderCache a bit to also check for that cache tag

But aside from that detail, I agree that making this rely on config might not be such a great idea. Your further clarifications I also tend to agree with. The suggested implementation here is one of "do not cache", but it doesn't mean that other implementations have to go down that same path.

Okay in that spirit I'll go over the MR again, but I would cautiously agree with the approach. We need to be very aware of #108, though. Ideally those issues get fixed sooner rather than later so the changes introduced here don't run havoc there, even if it's technically not "our fault" if it does.

The autoplaceholder config is config. We can change the defaults, but it wouldn't really apply to all existing sites that have customized this. IMHO that makes this very hard to control and those sites will then have negative effects from not having that.

I also think that cache tag bubbling is confusing with that special cache tag. I'm not exactly sure about exact behavior, but it seems unpredictable. As we learned recently, before ✨ Optimize placeholder retrieval from cache Active , placeholdered elements were still included if their element was already render cached, there might be other unexpected side effects.

Also, autoplacholdering doesn't disable caching like max-age 0 does, it would still be cached on its own.

It might not have been clear in the issue where we added it, but I already had this issue in the back of my head to use it here as well, that's why I moved it to the cache namespace and made it more generic. We should extend the docs to mention block plugins as well. In my mind, it's less about supporting CacheOptionalInterface in render arrays (we don't), it's supported specifically for block plugins and makes them not cacheable in the initial definition, but that's an implementation detail of BlockViewBuilder. Other places that support block plugins, such as layout builder, twig or block_field do not currently support per-block caching or placeholdering at all. Which, to reply to #107 as well, is IMHO OK, because it says "cache optional", not "must not be cached". In other words, "not worth caching on its own", which I think fits the use case in access policies.

Just adding this: I like how clean the current MR looks and I can see the reasoning behind it. But flagging the LanguageBlock as "cache optional" does not cover the load here. Because we want it to not be cached, ever. So it's hardly optional. Furthermore, we also want to placeholder it. And I can see that (placeholder & no cache) being the case for more pieces of content in the future.

Hence my suggestion for something that would both indicate we do not want to cache it and we want to placeholder it.

Quoting option B:

B) Use the new \Drupal\Core\Block\BlockPluginInterface::createPlaceholder() to force this to be a placeholder and cache it separately. In our projects, we're seeing 10k+ variants of this block, very rarely reused. That's MR !9798.

However, that MR seems to also involve CacheOptionalInterface? And I'm not sure we want to be using that here.

My reasoning is this: CacheOptionalInterface was introduced with access policies, where multiple smaller pieces will be cached together as a whole. In that context the interface meant: "If all of the smaller pieces indicate they are insignificant, we need not cache the whole."

However, render arrays with cache keys (including the language switcher block) aren't necessarily like that. They get cached individually and as part of a whole (DPC). So using the interface there seems ambiguous as it's not clear in which caching layer they would be considered optional.

Now, with that in mind, LanguageBlock should already be auto-placeholdered by virtue of its max age being 0 and BlockViewBuilder making it use a lazy builder. So setting createPlaceholder() should effectively change nothing about the current situation. The block will be placeholdered and DPC will not cache it.

So it seems the goal is to rid LanguageBlock if its max age of 0 and then make sure it doesn't poison the page. That part is not fully clear from the issue title and summary.

In that case, option B seems like the most straightforward fix, but we would indeed not want to individually cache all variations of LanguageBlock because it varies too much. So removing it's max-age 0 is a good thing, as it would otherwise kill all page caching when combined with 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed .

Then again, if we merely want to placeholder it and we don't want the placeholdered HTML to be cached, we have more options than to resort to max age 0. I'm wondering if this was ever explored. Let's look at renderer.config:

renderer.config:
required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']
auto_placeholder_conditions:
max-age: 0
contexts: ['session', 'user']
tags: []
debug: false

As you can see, we could achieve the very same result of having LanguageBlock auto-placeholdered by providing a cache tag that indicates the desired behavior. Much like the special "CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:" cache tag we have. This tag would also bubble up to FinishResponseSubscriber, but it would have zero effect on the max-age of the page.

So rather than use CacheOptionalInterface in a way that might confuse people, why don't we introduce a "SHOULD_AUTO_PLACEHOLDER_BUT_NOT_CACHE" cache tag, add it to LanguageBlock and renderer.config.auto_placeholder_conditions.tags?

Then all we'd have to do is adjust RenderCache a bit to also check for that cache tag and we'd have a system that works for ALL render arrays (with a lazy builder) rather than having to figure out ways to apply an interface to them.

Updated the MR with latest 11.x: solving conflicts and Fix test according with field ui API updates - issue #3386762 ✨ Use modal in add new field flow Active .

Updated the MR with latest 11.x: solving conflicts and Fix test according with field ui API updates - issue #3386762 ✨ Use modal in add new field flow Active .

Still an issue IMHO

Thank you for sharing your idea for improving Drupal.

We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Since there hasn't been a follow up going to close out, but don't worry we can always re-open!

Thanks all

@smustgrave Okay I'll check this out early-mid this week.

Trying to follow the steps believe I do see the issue
MR does appear to fix it,

Going to go on a limb

But @shiraz dindar mind checking the latest changes?

Just following up if still a valid task? If no follow up in 3 months could be closed out.

Thanks!

Since there has been no follow up in 3+ months going to close out, but don't worry we can always re-open!

Thanks all

Made change to test based on feedback.

Thanks for 1, makes sense, I added one small note on that.

For 2, it's not 100% explicit on the intent, but we do have performance tests that explicitly assert the page cache and dynamic cache page on umami, which is multilingual with language switcher.

We no longer have any changes in those tests because the behavior for them is unchanged in regards to the language switcher block, but earlier merge requests had changes there.

Tests are failing because of the new default of FALSE for hide_untranslated_menu_links in the menu block.

I think the upgrade path is now ready so removing that tag

I won't be working on this in the near future

Added the first test mentioned in #109.

Merged 11.x into the branch.

I agree with @catch so I applied the idea of the

OnlyShowTranslatedMenuLinksInterface

instead of checking for a specific class.

If the pipelines return green then I will make $context optional and default to null when not passed. I see no reason for making it mandatory

This was committed

Restoring meta data for the commit

Restoring meta data for the commit