🇧🇪Belgium @kristiaanvandeneynde

Tags often run magic, that's the whole point of the cache.bin tag: We collect these in a compiler pass and set some container parameters with this information. So what I'm suggesting wouldn't be 'arcane voodoo" because the tags suggest there is something doing some stuff to whatever is tagged this way.

I'm not sure this would be a Drupalism or rather a Symfonyism. I do believe this could reduce a lot of boilerplate when it comes to declaring your own cache backend.

We can choose not to tag the chain as a memory bin, that is an oversight because we used to need that where jsonapi had no proper way of declaring its memory cache backend. As I copied from there, I also copied over the double work.

Given how both inner backends are now properly declared, there is no longer a need for the ChainBackend to be declared as a backend itself.

We can still tag that cache as private again, I suppose.

That tags in the memory cache get invalidated twice is a bit unfortunate, I agree. But wasn't that always the case to begin with for the regular cache? Not saying one wrong justifies the other, just trying to point out that BackendChain is easily misused because it can take any backend regardless of how it was declared. Most caches you'd feed into it are persistent/static pairs where the persistent is tagged as cache.bin, also leading to double invalidation.

Perhaps we should open a follow-up to discuss these findings.

How about this?

no_longer_has_pattern_so_name_as_you_wish:
  class: Drupal\Core\Cache\CacheBackendInterface
  tags:
    - { name: cache.bin, bin: name_of_bin }

Where we use a comiler pass to set the factory and arguments, but only when the 'bin' tag is set and the 'no_factory' (or whatever) tag is not set.

Removes a ton of boilerplate from core cache bins, is completely BC and for those rarer cases that don't want a factory, they can opt out of using the factory.

Could even run a check if "class" is set and if not, polyfill it with: class: Drupal\Core\Cache\CacheBackendInterface. Then the standard declaration becomes:

no_longer_has_pattern_so_name_as_you_wish:
  tags:
    - { name: cache.bin, bin: name_of_bin }

Fix here looks similar to the latest one from 🐛 Sort out and fix language fallback inconsistencies Needs work , perhaps the two should be merged on the bigger issue?

Ran into another issue where we need to be able to deny access to missing translations (which we're currently using aforementioned workaround of unpublished translations for). Documented this on the unofficial parent issue: #2951294-63: Sort out and fix language fallback inconsistencies →

Should we go ahead and make that one the parent issue of all the other related issues, including this one?

We ran into another demand for disabling language fallbacks, adding that here to emphasize the need to be able to do this. Currently, entity repository adds the default language as a fallback and there is no way to stop it from doing so as it runs after the fallback hooks.

We are currently relying on the buggy behavior explained in #2978048-33: Unpublished translations should fallback to the original language → and #2978048-35: Unpublished translations should fallback to the original language → as at least it is some way to make Drupal not use fallbacks. All we have to do then is make sure that whenever a new content entity is published, we create unpublished translations for it.

It would be far superior for Drupal to be consistent across the board and to allow developers to choose whether or not they even want fallbacks to happen. It's perfectly valid to say: "Deny access to translated versions of a node until it's translated and ready". Especially in legal contexts where you are not allowed by law to show certain content outside or inside of certain countries.

Our current work-around doesn't work perfectly, because ER field, the entity_upcast operation and the default entity_view operation all behave differently.

valthebald → credited kristiaanvandeneynde → .

All green!

Ideally I add another test case for that last bit I just added. Don't have time right now, but hopefully over the next week or so. Shouldn't take too long but this week is really busy.

All green, now to add a bit more hardening. May change my mind but let's see what this catches.

Blocking issues have been fixed, simplifying MR and seeing what testbot thinks. Might add a little bit more hardening after tests go green.

+1 from me, can confirm tests went green with the rename in place.

Yeah it was a bit premature to start on this one already, the issue is merely here for when we start working on D12. hence the tag.

Follow-up for D12 here: 📌 Enforce second parameter in BreadcrumbBuilderInterface::applies Active

kristiaanvandeneynde → created an issue.

• Created a CR here: https://www.drupal.org/node/3459274 →
• Updated the code to account for BC
• Addressed all feedback

Only needs a follow-up to change the signatures in D12.

I'd go with Third-Party for two reasons:

1. We already have that naming pattern for config
2. While you're right that the implementing module doesn't set the data like they do with field_ui_base_route, it's arguably still a third-party piece of information that more often than not is added via a hook_entity_type_build/alter. The Field UI base route is more the exception to the rule than the rule itself. I wouldn't get too hung up on that.

Will try to fix these minor issues tomorrow. In the meantime, I answered the question regarding cache operation counts.

So couple things for other people reading this:

• Cache contexts are supposed to be fast, so that we can load variable things without having to run too much code to figure out the cache ID in VariationCache::createCacheIdFast()
• If your cache context needs to load something (e.g. user roles), that needs to be cached (e.g. entity cache) so your next return value can be calculated quicker
• Trying to make convertTokenToKeys run fewer requests to the cache contexts is, as pointed out in the IS, dangerous territory as the return value may have changed over the course of the request

With that out of the way, I need to point out that this is no longer a Group-specific thing, but a core thing. We now have Access Policy API and, just like Group, it uses a persistent and memory variation cache to store your calculated permissions. So if your permissions are highly variable, we have a bunch of cache contexts to check whenever a permission check is ran. As far as that goes, I would definitely focus on making sure cache contexts are fast.

For the specific case of only having a single cache context (I'm profiling with anonymous user, it's user.roles), running optimizeTokens() seems pointless. It will never be able to optimize a single context away, so we could add an early return?

Agreed, we could make optimizeTokens return early if the count of the array is smaller than 2.

A static cache in convertTokensToKeys? but we'd need to understand if and when context values can change during a request.

Very dangerous territory as explained above.

Using VariationCache as a static cache like that is obviously fairly expensive

Yeah, but it's required for access policies to work. I think the cost-benefit is warranted here given how much is now possible with the access policy API.

group specifically always collects 3 different kinds of things in \Drupal\group\Access\GroupPermissionChecker::hasPermissionInGroup() but will always only use 1-2 of those and never all 3.

Yeah, fully agree we can optimize that in Group.

As we discussed in person, looking good but let's make it more defensive and clear that the negotiator is internal

Hey, sorry for being unresponsive. I'm currently at Dev Days and not keeping an eye on my issue queue as much. Thanks for committing this, though!

Could you file a new issue about this please? If core is setting that much data through drupal_js_cache_files, that definitely needs a dedicated issue.

Okay so a local attempt at getting this to work with entity types dynamically was running into eternal loops, but I just had a chat with @catch and we think we may have a solution for that:

We fire an event early on that asks for this list of cache tags, when/if we find this list in the bootstrap cache we set it on the container. In the CacheTagsChecksumTrait, we check for said parameter and don't do anything special if it's not there. This will prevent eternal loops.

Then, we can have the entity type manager load all the info it needs and spit out a list of common cache tags. In the end, we cache said list without cache tags (because here be monsters), but with a max-age in the bootstrap cache. Because it's just an optimization, we don't need to worry that much if this cache becomes stale for a short while; next cache clear or after the list expired it will be recalculated anew.

Okay I see what you're getting at now. I would approach this slightly differently, so that we don't have to repeat this code if we care about the cacheable metadata. Domain has a pretty good class/method names for this that we can mimic.

Something like: GroupSitesNegotiator::getActiveGroup($cacheable_metadata)

public function getActiveGroup(CacheableMetadata $cacheable_metadata): GroupInterface|null {
    $cacheable_metadata->addCacheTags(['config:group_sites.settings'])
    $context_id = $this->configFactory->get('group_sites.settings')->get('context_provider');
    if ($context_id === NULL) {
      return NULL;
    }

    $contexts = $this->contextRepository->getRuntimeContexts([$context_id]);
    $context = count($contexts) ? reset($contexts) : NULL;
    if ($context) {
      $cacheable_metadata->addCacheableDependency($context);
    }

    $group = $context?->getContextValue();
    if ($group && !$group instanceof GroupInterface) {
      throw new \InvalidArgumentException('Context value is not a Group entity.');
    }

    return $group;
}

Then we can inject that into GroupSitesAccessPolicy and use that there.

The problem is that when you get to that function all queries should have the right metadata. We've previously had an incompatibility with VBO because it stripped important metadata from queries. So my advice would be to see what the CSV export module is doing to the Views query.

In order to drop dependencies I need to release a new major (Group 4). My current goal is to release 3.3.x/2.3.x with an upgrade path from 2 to 3. Then we can start working on v4 where we drop all the dependencies. The new Group release can already depend on FP v2, which would be a trivial change to make.

Just for clarity: Whatever is cooked up here needs to make sure this is about the request as the IS clearly states. There are sub-processes, such as the access policy processor, that have their own cacheability layer which should never make it to the request.

So a collector that collects any 'addCacheContexts()' and similar calls would be too greedy.

You're not supposed to use it in a block or controller. You write an access policy and that affects the total set of permissions someone has. Then ALL permission checks across the entire website will respect that list of permissions.

It doesn't mention the two methods you list because those are part of the base class and hardly ever needed for policies that add to the set of permissions. If you inspect the AccessPolicyInterface in any proper IDE, you can see that it has multiple example implementations in the tests.

We could automatically check the box for core "this issue should count for credit" but not for contrib.

Yeah, having an extra "this issue should count for credit" flag as opt-in would be nice here. It would allow you to still credit the people who worked on an issue, but at the same time flag said issue as "this shouldn't go towards any marketplace rankings" to discourage the spam of readme fixes and the like.

Also, if you want it to be nicer you could also have a checkbox unchecked by default saying "This issue warrants credit", but obviously a more descriptive label and description. You get the idea, though.

Either way:

• No credit should be given, checked by default
• Credit should be given, unchecked by default

Any of the above would require active consent of the maintainer to provide credit. If we have that and we still find a bunch of non-issues being flagged as credit-worthy, it proves intent of gaming the system and we can take action far more quickly against the abusers.

From a thread on Slack:

The only issue I have with repurposing the priority field is that it could arguably be something people can hide behind. "Oh, it didn't use to mean that" or "I don't consider this a minor thing" and that anyone can change it.
You could catch people messing with said field, but the fact that it defaults to normal rather than minor would still lead to a lot of these non-issues getting more credit than they should.
Having a new maintainer-only field would make it really clear what the intent is and we could easily run some queries against that to find abuse.

In this case a checkbox could be: "This issue was trivial enough that it does not warrant any credit, e.g.: Fixing punctuation, spelling error, minor README changes, etc."

Then check it by default, making it a deliberate opt-out for maintainers when they accept a MR. If we see these being opted out of on the spammy issues we've been suffering from for the last few years, we can immediately take action because it clearly shows intent of gaming the system.

kristiaanvandeneynde → created an issue.

If you need help figuring out why, let me know.

Should the manager just include that by default and this change would only be used for more exotic cases?

It could, but just like you I'd argue that that is out of scope. For now I would focus on fixing the bugs caused by not correctly setting any cacheable metadata when the applies() check returns FALSE.

No worries :) I'm not a native speaker either but I tend to want to get the grammar right, so I tend to look these things up when in doubt. You did make me think, though, and I believe the simple present is preferred here, as the implementation is arguably still to be written at the time of reading the documentation.

Will change that little detail and leave at RTBC.

Can't the "to" be removed?

That would make it broken English. You have the infinite "to add" and then "to" as you're adding to something (the CacheableMetadata).

Or specify to what we're adding metadata?

This documentation is for the $cacheable_metadata parameter, so I think it should be clear that that's the thing you're adding to?

Anything you specified here does not have to be repeated in the build() method as it will be merged in automatically.

You can use "Anything you specify here" or "Anything you specified here", depending on whether you want it to read like a forward-looking suggestion or vice versa. I wouldn't use "have", though. If you want we can change it to the present tense.

As for "merged in", it's because you merge whatever was specified into the final result. Because that sentence has no mention of "the final result", the "to" part of "into" is dropped. Example: John was about to walk into the river, as he walked in, he felt something sharp beneath his feet.

Hopefully this is a bit easier on the eyes.

@smustgrave Updated the IS to reflect the actual bug and how it should be fixed. The initial IS was just one case of running into it, but 🐛 VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Active exposes the problem in more detail as it also causes incorrect cache redirects to be written.

Please don't convert MRs into patches as it creates a lot of unnecessary noise. Especially when the MR is still up for review and might change.

Feedback addressed.

The MR is the correct approach, it's just that the issue summary needs severe updating.
The new approach is #10, #36 and #37

In essence: Allow the applies() method to set cacheability. Because the breadcrumb builders run in the same order every time and because only one can build the actual breadcrumb, we don't need to care about ALL builder's cacheable metadata, just the data up until the builder that actually applies.

I'll try to update the IS tomorrow.

Should go green now. From the other issue:

This caused StandardPerformanceTest to fail as there are now 2 more cache gets, which makes sense because we added a cache context.

Updated MR. Feel free to request we remove all occurrences of url.query_args:foo, but as both you and I concluded it might not be so bad to leave them in.

Thanks for the detailed review, I'll rename the subscriber as jsonapi has that class marked as @internal so we can do that.

This also kinda triggers me, why would we keep it then?

If, for some reason, jsonapi ever decides to remove the validating subscriber, the rest of the code would remain functional and safe. It's also not clear from said code that there is a subscriber somewhere making the omission of these cache contexts possible. So for those two reasons I'd keep them: clarity and future-proofing.

If, however, you are 100% sure the validator will never be removed, then by all means we can get rid of the cache contexts. It would be a bit cleaner for sure.

The only other way that I see is that we introduce a new applies() method on a new interface that can return something like AccessResult. A binary result + cacheablity metadata.

This was my first thought here - new interface, extend the old interface. We could also pass in an optional CacheableMetadata object into the applies method so the return doesn't need to change.

Why note use the deprecation policy guideline on introducing a new argument to an existing signature? https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... →

I'll try that approach to allow a 2nd argument to be passed to applies().

Yeah I'd rather have them also approve this as it's completely in their area of core :)

All green, don't suppose this needs extra tests as the current tests have been updated to reflect the new expectation of always seeing url.query_args in there.

As far as the two possible outcomes go, I would rather we just varied the request by url.query_args. Reason being it would otherwise introduce a cache redirect for only url.query_args:_format, only to be followed by another cache redirect for url.query_args, url.query_args:_format for ALL scenarios where the _format query arg isn't present.

That is just not enough of a use case to warrant the extra cache redirect (and thus cache get). As soon as someone adds the _format query arg once and sees the exception, they're going to remove it from their application and it will never show up again. So reserving a redirect just for the _format arg is overkill.

It's also good because then all of the url.query_args:FOO cache contexts being added across jsonapi would no longer be necessary as every jsonapi request and response now varies by url.query_args. It's harmless to leave them in, as I would even argue it's extra hardening for if we ever remove JsonApiRequestValidator, but as the tests will show, they will have zero effect as long as the validator is in.

Can we still reach out to whoever wrote this to hear why the whole concept of groups was introduced to begin with? I feel like another solution might be to just drop the whole idea of groups.

From what I can recall (from 8 years ago, mind you), the whole group thing is so you could specify an ER field where you don't care about the selection plugin, as long as it's tailored towards e.g. nodes. Then it would default to whatever core has, but if someone else came up with a "better" selection plugin, that one would be used instead.

In practice, I have yet to see anyone make use of that because often the one creating the selection plugin is the one consuming it. E.g.: I have a really specific group role selection plugin in Group that I do not expect anyone to ever "improve".

kristiaanvandeneynde → created an issue.

I know it doesn't apply, that's the whole point :)

some tests are still failing as I can see, needs work?

Those fails stem from my "fix" for breadcrumbs. The actual fails caused by the VC changes are all green now. If we split off the breadcrumb and jsonapi fix everything will be green.

Okay so this shows that there's only two detectable bugs in core right now:

CommentBreadcrumbBuilder::applies(), I "fixed" this the wrong way here to make tests go green. This caused StandardPerformanceTest to fail as there are now 2 more cache gets, which makes sense because we added a cache context. It also made phpunit tests report an unknown failure.

JsonApiRequestValidator::validateQueryParams(), I fixed this properly here but it needs a dedicated issue. Will create that one next.

There might be more core bugs, such as the one we just fixed in 🐛 Insufficient cacheability information bubbled up by UserAccessControlHandler Active , but we don't always have tests that are extensive enough for this VC fix to throw exceptions there. I.e.: They are flying under the radar and this exception will over time throw an exception on sites that have warmed their caches enough to run into the buggy code.

Fixed it. JsonApiRequestValidator::validateQueryParams() only sets url.query_args when validation fails, but obviously it also needs to set that when validation succeeds. Because if the query args change, so can the validity.

Current findings:

1. DPC sets the initial cache contexts of 'route' and 'request_format'
2. This ends up at a redirect to 'url.site', set by JsonApiDocumentTopLevelNormalizer::normalize()

Then, the first time we try to store the return value of JsonApiRequestValidator::validateQueryParams(), leading to the creation of a second CacheRedirect, pointing to 'url.query_args'

But the second time, we try to store the return value of EntityResource::buildWrappedResponse(), which adds 'url.query_args:fields' and 'url.query_args:include' and somewhere along the line 'user.permissions' also makes it in there. Probably an access check, but have to figure that one out.

So the redirect at address response:[request_format]=api_json:[route]=jsonapi.entity_view_display--entity_view_display.individual_SOME_HASH_1:[url.site]=http://web used to point at 'url.query_args', but now points to those new cache contexts leading to the final cache ID of:

response:[request_format]=api_json:[route]=jsonapi.entity_view_display--entity_view_display.individual_SOME_HASH_1:[url.query_args:fields]=:[url.query_args:include]=:[url.site]=http://web:[user.permissions]=_SOME_HASH_2

This means that whatever code comes after JsonApiDocumentTopLevelNormalizer::normalize() is currently inconsistent in setting cache contexts.

Well the main issue is that Subgroup is based on the nested set model for fast reads (and slower writes as a tradeoff). Theoretically it could be done if we made the tree metadata fields multi-value and tagged the same groups as belonging to multiple trees.

However, the entire module was built around the single tree principle and therefore this would constitute a significant rewrite of seemingly unrelated systems. Most of these rewrites would probably end up breaking BC, so it's likely that an entire new major version is needed for this functionality.

Pushed a proof of concept. Keep in mind that these helper functions slightly differ from how andIf() and orIf() behave internally.

• In the original logic, if the original result is uncacheable, then the other result is checked for cacheability and merged if it could turn an uncacheable result into a cacheable one.
• In the new logic, we simply do not run the extra code or add the extra cacheability if the original result will not be affected by the closure result.

The whole point was not to run dead code or add dead cacheable metadata. The trade off being that we may end up not trying to make uncacheable results cacheable, which I assume is an edge case that would occur far less than the current situation where cacheable metadata is needlessly added.

Starting to think we need:

• andAllowedIf(\Closure $closure)
• andForbiddenIf(\Closure $closure)
• orAllowedIf(\Closure $closure)
• orForbiddenIf(\Closure $closure)

Perhaps also andAllowedIfHasPermission(string $permission) and orAllowedIfHasPermission(string $permission);

Then the IS would look like:

$result = AccessResult::allowedIfHasPermission($account, 'access user profiles')->andAllowedIf(function($cacheable_metadata) use ($entity) {
  $cacheable_metadata->addCacheableDependency($entity);
  return $entity->isActive();
});

All of these would internally do the sanity checks where the other code does not have to run and only then yield to andIf() and orIf() with the result of the callable. This leaves andIf() and orIf() intact whereas the original thought was to get rid of them, so maybe we need to revisit that or make the original methods protected or private.

This will need a bit more love. If we simply use andIfClosure and orIfClosure, we can only avoid calling the closure if the original access result was forbidden. Any other scenario requires comparing the current to the other result, even if we may end up not using the other result. In the current proposal we'd still have to call the other closure to do some inspection.

So perhaps we need something that can signal intent. The example code in the IS could also not call the callable when the original result is N, because it knows the outcome of the closure cannot be F and therefore has no impact.

kristiaanvandeneynde → created an issue.

What would you suggest? We put that in a service instead? Something that could return the Group detected by Group Sites? I'd be up for that, but I'm a bit strapped for time. Feel free to open an MR with your suggestion so we have a starting point for discussion.

kristiaanvandeneynde → created an issue.

Could you provide steps to reproduce on a clean install?

How did this get set back to active? o.O

Could you try checking out the latest Group 3.3.x using git? If you use composer you'll have to specify the -W flag to also download all dependencies, including Drupal 10.3-rc1

Thanks, looks good to me.

Sorry I should have been more specific: Test code that shouldn't at first sight have been affected.

if ($sparse_fieldset === NULL || !empty(array_intersect(['mail', 'display_name'], $sparse_fieldset))) {

Could you briefly explain why this is needed now?

Could you also elaborate on the mitigating lines? Why weren't they needed before and are they now?

What about the fix you copied from REST?

Changes look great, would RTBC but I think we should explain why the seemingly unrelated tests were changed. If you could add a few notes with your findings there, then all good to me.

Yup, mistakes happen. Glad we figured it out.

Are you sure though? AccessResult::andIf() has 3 branches, we are dealing with the third.

    if ($this->isForbidden() || $other->isForbidden()) {
    }
    elseif ($this->isAllowed() && $other->isAllowed()) {
    }
    else {
     // WE ARE HERE WHEN ALLOWED+NEUTRAL.
    }

In said branch, we pass the following check:

      if (!$this->isNeutral()) {
        $merge_other = TRUE;
        if ($other instanceof AccessResultReasonInterface) {
          $result->setReason($other->getReason());
        }
      }

So when you have allowed and use andIf to feed in a neutral, you should get the cacheability from the neutral access result. This makes sense because A+N could turn into A+A under different cache context values, and that would lead to a different outcome (Allowed).

However, when you have neutral and feed in allowed, then the second one doesn't get merged in. But that makes sense as the first one is N and we know the second one can't be F. N+N and N+A both return N based on the first N, so why even bother with the second result?

So I don't see a bug in the code. Am I missing something?

It's ready to test now. You don't need D10.3 if you use drush updb

kristiaanvandeneynde → created an issue.

Re #10: Yup that's exactly it. You make sure your 2.x.x is on the latest version and then change your code to the latest 3.3.x with this MR applied. Also make sure you check out the 3.x.x-compatible code for any Group related modules you have installed.

So far I've heard that this breaks with the Subgroup (Graph) → module because they changed their plugin machine names between v2 and v3 and so that module would also need an upgrade path. Any other module such as Subgroup, Group Sites, etc. should theoretically be fine.

If the intent was to manually optimize 'user.permissions' into 'user', don't. VariationCache needs all of the cache contexts, even if they will eventually be optimized away because, during said optimization (cache context folding), extra cache tags might surface that need to be added to the cache entry.

So in this case, the first if-statement can vary by user.permissions, the second by ['user', 'user.permissions'] and the third one too. Again, do not manually optimize this, you're introducing bugs and potential security issues.

These are coming from JsonApiRequestValidator::validateQueryParams() and EntityResource:buildWrappedResponse() in that order. So probably also some check not making it into the cache contexts.

Seems like the other bug is with jsonapi:

Trying to set a redirect at an address that already had a redirect with nothing in common, old redirect at address "url.site, route, request_format" was pointing to "url.query_args", new redirect at same address points to "url.query_args:fields, url.query_args:include, user.permissions".

The data it was trying to set:

\Drupal\Core\Cache\CacheableResponse::__set_state(array(
   'headers' => 
  \Symfony\Component\HttpFoundation\ResponseHeaderBag::__set_state(array(
     'headers' => 
    array (
      'cache-control' => 
      array (
        0 => 'no-cache, private',
      ),
      'date' => 
      array (
        0 => 'Mon, 03 Jun 2024 16:26:31 GMT',
      ),
      'content-type' => 
      array (
        0 => 'application/vnd.api+json',
      ),
    ),
     'cacheControl' => 
    array (
    ),
     'computedCacheControl' => 
    array (
      'no-cache' => true,
      'private' => true,
    ),
     'cookies' => 
    array (
    ),
     'headerNames' => 
    array (
      'cache-control' => 'Cache-Control',
      'date' => 'Date',
      'content-type' => 'Content-Type',
    ),
  )),
   'content' => '{"jsonapi":{"version":"1.0","meta":{"links":{"self":{"href":"http:\\/\\/jsonapi.org\\/format\\/1.0\\/"}}}},"data":{"type":"entity_view_display--entity_view_display","id":"643f166a-a78e-4bad-941c-493d34d10dff","links":{"self":{"href":"http:\\/\\/web\\/jsonapi\\/entity_view_display\\/entity_view_display\\/643f166a-a78e-4bad-941c-493d34d10dff"}},"attributes":{"langcode":"en","status":true,"dependencies":{"config":["node.type.camelids"],"module":["layout_builder","user"]},"third_party_settings":{"layout_builder":{"enabled":true,"allow_custom":true}},"drupal_internal__id":"node.camelids.default","targetEntityType":"node","bundle":"camelids","mode":"default","content":{"links":{"settings":[],"third_party_settings":[],"weight":100,"region":"content"}},"hidden":{"layout_builder__layout":true}}},"links":{"self":{"href":"http:\\/\\/web\\/jsonapi\\/entity_view_display\\/entity_view_display\\/643f166a-a78e-4bad-941c-493d34d10dff"}}}',
   'version' => '1.0',
   'statusCode' => 200,
   'statusText' => 'OK',
   'charset' => NULL,
   'cacheabilityMetadata' => 
  \Drupal\Core\Cache\CacheableMetadata::__set_state(array(
     'cacheContexts' => 
    array (
      0 => 'url.query_args:fields',
      1 => 'url.query_args:include',
      2 => 'url.site',
      3 => 'user.permissions',
    ),
     'cacheTags' => 
    array (
      0 => 'config:core.entity_view_display.node.camelids.default',
    ),
     'cacheMaxAge' => -1,
  )),
))'

Will try to debug that one further when I have time, but that might be Thursday at the earliest.

This surfaced as part of some extra hardening I'm trying to introduce into VariationCache: 🐛 VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Active

Re #11:

Does this mean we need to run through all breadcrumb builders?

No, because they always run in the same order. So as long as we clear the cache when a new builder is introduced into the list, we're fine.

Let's say you have 3 builders, the 1st applies() check varies by route, the 2nd by domain and the 3rd by user roles (for whatever reason). Then, as long as the first in the list applies, all we need to care about is the 'route' cache context. Only when the first one ever fails and we fall back to the second one, do add the url.site cache context. At this point, VariationCache will write a CacheRedirect at the current value for 'route', setting the 'url.site' cache contexts. Then, if both the 1st and 2nd applies() check fail, will another CacheRedirect be created to allow VC to store items from the 3rd builder.

Found an issue that reports this, moving on.

Yup, from CommentBreadcrumbBuilder:

  public function applies(RouteMatchInterface $route_match) {
    return $route_match->getRouteName() == 'comment.reply' && $route_match->getParameter('entity');
  }

This conditional applies return value is never represented by the 'route' cache context when the result is FALSE.

I'm sure we'll find plenty similar core bugs while combing over the test fails here.

Okay so figured out where these are coming from: CommentBreadcrumbBuilder::build() and PathBasedBreadcrumbBuilder::build(). It seems like either one or the other fires, but not both, leading to the exception we introduced here as the same cache ID now triggers a completely different redirect.

CommentNonNodeTest message was:
Trying to set a redirect at an address that already had a redirect with nothing in common, old redirect at address "languages:language_interface, theme, user.permissions" was pointing to "url.path.parent, url.path.is_front", new redirect at same address points to "route".

Sucks how browser tests obscure these, so far I always put the following in these tests so I'd happily learn about a better way of debugging this:

    $this->assertEquals('foo', $this->getSession()->getPage()->getContent());

Oof, I was afraid we'd see this many test fails. Let's start digging, I suppose.

Okay tests have thorough coverage now. Let's see if this breaks core and why.

MR needs work, first item in the chain is always the stored item or a redirect, so we can't have this "we already checked initially" stuff in there. Working on it.

Updated IS to shorten list of remaining tasks.

Opened 🐛 VariationCache needs to be more defensive about cache context manipulation to avoid broken redirects Active , haven't opened an issue for the UserAccessControlHandler bug yet as I'd like to see how many more surface once the VC MR is ready.

Marked as major because we have a few sleeper bugs in core that this might uncover.

kristiaanvandeneynde → created an issue.

Added a note to the BC policy → and the documentation →

Added a note about access policies being final