I love the way this issue evolved and the end result is really simple and elegant. Thanks to everyone involved!
Also +1 for @quietone's concerns regarding change records, it would be nice if we could flag them for removal if throughout the lifetime of an issue it becomes apparent they are no longer needed. I couldn't find an issue for this in the issue queue → , but as I was searching through it I found your comment here (on the same day as your last comment in this issue): ✨ Publish change records when issues are fixed Postponed So I guess that issue can be used for that discussion.
Anyways, big +1 from me.
I'm actually on the fence if we even want the new test in core. It's pretty clear there was a mistake in the entity type annotation and link generation should already be well covered. I'd treat the test as a test-only thing for us to verify that everything is now fixed and leave it out when committing this.
Still RTBC and approved either way, just a note from a maintainer to a committer.
Seems straightforward enough for me to sign off on this. Thanks!
I have already addressed this on Drupal Slack. I even fixed something in there: 🐛 BookBreadcrumbBuilder needs to always set the route.book_navigation cache context Fixed
The point is that the tests now fail if your cache contexts are broken or insecure. This was the whole point of this issue. While we shied away from straight up breaking existing sites by throwing exceptions, the compromise was that we would be breaking tests instead to show people something was wrong.
Tests broke in Book, I had a look and fixed it. There may be more to fix, but that is really a bug in Book and not core.
Re #199 please stop spreading this lie:
AccessPolicyInterface.php doesn't explain what's a $scope or a policy
You disrespectfully complained about it before on Slack and when I pointed out that you had failed to read the official documentation → , you deleted the thread. As I mentioned in said thread:
Access policies are considered internal code, so their in-code documentation is limited. The AccessPolicyProcessor, however, goes into great detail in the code comments.
If you want to stop working on something for whatever reason, that's your prerogative, but don't drag other people's work into it.
On the subject of doxygen, I do agree that some aspects of core are more documented in code and others are more documented on the website. If you want to see that changed, you could create (or find) an issue for it, link to it here and then use that as a starting point to figure out whether or not this issue's MR should already adhere to what's being discussed.
Again, if you'd rather not have that discussion and stop working on this MR altogether because of it, that's also fine. No-one is forcing anyone to work on something, we're mostly all volunteers here.
I do :D
Looks like we have unrelated test failures. Either way, the fix in the MR makes VariationCache stop complaining, which was the goal of the issue. I agree with @mxr576 that we could opt for better wording of the @todo messages.
kristiaanvandeneynde → created an issue.
The thing is, and I believe Berdir hinted at such a thing earlier in the comments, all systems need to know about your custom value you are trying to pass to the cache context.
With the access policy processor it's fine because the processor itself receives an account and we can use the account switcher there if someone uses a user-based cache context. It also passes the account object around so the individual access policies don't have to rely on the current user service (but could, if they wanted to). But we don't have this global representation of a value like "current user" for everything and neither do we have switchers for all the things.
Another thing you would end up with is that pages would become completely unchacheable or bogged down if we start using cache contexts that do not rely on a global context value. Keep in mind that cache contexts need to be fast. If we start bubbling up a bunch of cache contexts that need to load a few entities to get their return value, we'd be getting really slow pages.
Having said that, I am in favor of trying to rethink the cache contexts we have to allow for better reusability. Currently, what we're seeing in this issue is that we have a system which is given an account and then caches by a cache context which does not know what that account is and assumes the current user. The APP can work around this because it controls the caching, but the AccessResult does not so it can't.
So what we would need here is a calculated cache context called user.permissions:%uid, which would default to current_user if nothing was given and the permission hash of the account if provided. Then the cache context could have an optimization where it compares the passed in account ID to the current user and runs with the latter if the IDs match.
The issue is that if you have user.permissions and user.permissions:%uid on the same page, they will get folded into user.permissions and that crucial piece of information (the UID) gets lost.
Which brings us to the next possible solution: current_user.permissions and user.permissions, but that would require all of core and contrib to rename their user.foo cache contexts to current_user.foo. And you'd still risk folding issues as I'm not sure CacheContextsManager::optimizeTokens() currently knows how to deal with user:13,user.permissions:14 or user:13,user.permissions:13. At a glance it seems it would not work.
So hopefully it is starting to become clear what a pain in the ass it can be to tell a cache context which specific value to use for all ancestors of a given cache context.
Which brings us back to reusability (again): If we could somehow say: "I need to know the cacheability of a given context, provided I swap out its global value for something else.", we'd be in better shape. For user.foo we can using the account switcher, sure, but we need a universal way for all cache contexts like route, url, etc...
If we allowed cache contexts to declare what they depend on, we could write a service which allows you to call said cache context's methods while in a mocked environment so to speak (different route, user, url, ...), without actually changing the environment like the workaround in APP.
Solve that, and you solve this whole issue and more. But keep in mind the most preferred option is the one that doesn't require all cache contexts to be rewritten, even though I see no other way around it.
Found 2 nits, everything else looks okay. The only thing that still has me worried a bit is the jsonapi tests becoming uncacheable. It might very well be intentional and therefore harmless, but it would be great if we could git blame, find the original issue and then ask around to make sure why a NULL access result is allowed and added as a dependency (therefore setting max age to 0).
Awesome, thanks!
Moved from VariationCacheTest to VariationCache and added the following to it to make it even more clear:
// Another way this might happen is if a new object that can specify
// cacheable metadata is instantiated without inheriting the cache
// contexts of all the logic that happened up until that point. A
// common example of this is when people immediately return the
// result of one of the factory methods on AccessResult, without
// adding the cacheability from previous access checks that did not
// lead to a value being returned.
Also this no longer depends on other parts of core being fixed, so review bot can return.
From #45:
PHPUnit has dropped support for expecting errors, it hasn't stopped failing on E_USER_WARNING, so the dropped support only makes it a pain to test the warning itself, not to get a test failure on an unexpected warning.
This definitely seems like a duplicate of 🐛 Database update errors when upgrading from 1.5 to 2.1 pluginid not found Active
Thanks @unqunq!
Yeah I switched to E_USER_WARNING to be sure.
Updated the MR, follow-up here: 📌 Convert trigger_error in VariationCache into a LocigException. Active
kristiaanvandeneynde → created an issue.
The E_USER_ERROR is only temporary, though. Will be removed in Drupal 12.0.0 and then we'll throw a proper exception. Your issue seems to also have a MR targeted at D12, so I suppose we're fine? I adjusted the MR, will push in a second after creating a follow-up to point to.
Okay, let's do #44. Seems like a really nice temporary solution until we can switch to exceptions.
On the surface level, I like these suggestions. If you were to work on them and provide before-and-after performance results, that would make it easier for me to accept the changes. We now have PerformanceTestTrait in core, which allows us to measure how expensive a page is, but that only works for scripted page visits. Seeing raw test data from a real-world example is always a nice thing to cross-reference.
Okay, did some digging and following 📌 [meta] Replace calls to ::expectError*() and ::expectWarning*() Active it seems we actually changed errors into exceptions in other places in core. Which means that, there too, existing code could start breaking.
With that in mind, would it be fine to just commit the MR as is (with the exception) and deal with the fallout as people stumble upon broken/unsafe cache context assignment? Keep in mind that a site which violates the thing we're detecting here will have broken caching and potentially even a security issue, so maybe we have to break loudly to detect these flaws ASAP?
Looks like we can no longer test for E_USER_ERROR. It's still there in PhpUnit 10 (even though it claims otherwise), but will be removed.
/**
* @deprecated https://github.com/sebastianbergmann/phpunit/issues/5062
*/
public function expectError(): void
{
$this->addWarning('Expecting E_ERROR and E_USER_ERROR is deprecated and will no longer be possible in PHPUnit 10.');
$this->expectedException = Error::class;
}
So now what? :/
While sometimes I would agree with a multi-tiered approach, here I'm not a fan. I'd like to keep the VariationCache as lightweight as possible and introducing all these options seems to complicate matters even more for a piece of core that few people understand. If we can trigger_error() with E_USER_WARNING and make tests fail on those, we're already in the sweet spot IMO: Tests will fail, but sites will not.
Then, in a next major we should turn the trigger_error into exceptions because we really want to prevent people from setting incorrect cache contexts. It would stop a lot of potential security issues dead in its tracks before they can even get a chance to make it into core/contrib.
The thing is we need tests to fail if they uncover incorrect (or dangerous) cache context use. I'm not sure we can achieve that for all types of tests if we switch to error logging.
Existing vs new sites shouldn't be a cut-off point as, if there is a bug in core, it would mean people couldn't install new Drupal websites whereas existing ones would continue working.
At this point I'm content with the new tests. We could always add more and more to prove things work, but right now we have extra test coverage for when things don't work. Which is what this issue was about.
Now final concern is how do we proceed? I think it was Alex Pott who raised the question that we cannot be throwing exceptions for things we previously allowed people to do wrong. The whole reason this MR started with exceptions was to catch the places in core that were broken and we've since fixed those in the spin-off issues.
Yet @mxr576 has an issue lined up which depends on this one so we can easily tell if his DPC changes open up even more cans of worms. So he would definitely benefit from this MR introducing the hardening as exceptions.
We could set a flag somewhere and log an error when its one value and throw an exception when its the other, but then the VariationCache service would need to get a logger injected only for us to remove it again in a next major version. So it would instantly be deprecated.
Any thoughts?
I am working on the final comment and reverted the extra hardening locally. Just making sure we have ample test coverage.
We never touched config overrides in that issue. The only thing that has changed is that we cache the outcome of your permission calculations, including the ones coming from roles, because that's the only way to safely do so.
Altering permissions live is a security risk, the code in the IS is unsafe. I specifically designed the Access Policy API because I too needed alterable permissions but found out that dynamically changing what permissions a role has is an attack vector. I shall not go into further detail on that matter because you apparently have such code in your system.
My advice would be to introduce an access policy to take care of whatever logic you had in your config overrides. Then it will work and be safe.
Updated the comments, thanks for the review
Yeah I can see where the confusion is coming from. Wrote a quick update.
I need to update the IS to reflect the latest MR, but the explanation still stands:
AccessResult::andIf() and ::orIf() each have possible combinations that make the passed in access result obsolete. This leads to whatever was used to build the second access result to run for no reason at all. In case of chained calls, it may even lead to the second check's cacheability making it into the result even though the second check never applied.
Could we achieve the same thing through a deprecation dance which changes the parameter types but not the method names?
You mean check if the passed in argument is of type Closure and then run the new code, otherwise run the old code with a deprecation warning?
Okay we are in agreement now. So if a committer could review, we can add tests when the approach is approved.
@smustgrave we had another round of feedback. I am on the fence about two changes (predicate name and a more explicit param documentation.
Once @mxr576 and I agree on the final open issue, we can apply the 3 suggestions and this can be considered accepted and we can add some cases to AccessResultTest
Agree with the counterpoint made in #4, even though we do have "vultures" there. Plenty of them too. I guess we need to keep the generic issue queue around even if it's an attack vector so to speak.
On the main subject: My main point is that I'd rather we spend resources on completely voiding credit gained through spamming issue queues and then see where that lands us. For all we know these companies might stop spamming altogether.
If they still spam the issue queues regardless of the fact that they no longer gain credit for it, then it's a cultural issue and we need to figure that one out. At that point I wouldn't mind spending resources on allowing maintainers to block these companies.
While I absolutely dislike what some of these companies are doing, I'm not sure if this is a good idea. Some downsides off the top of my head:
- We'd be spending dev resources to allow us to block organizations, while we could spend said resources in implementing a suggestion from ✨ Consider weighting issue credits by issue priority when ranking Marketplace organizations Active . I feel strongly for the suggestion I made there as it would reduce the amount of credits gained from these low-effort issues to zero, effectively removing all incentive to game the system.
- It enforces the idea that certain companies are lost causes. While I would agree for some of the worst offenders, it may lead to situations where a frustrated maintainer prematurely blocks a company even though they had no intent of gaming the system.
- It creates an avenue for conflict. We have a system in place where @hestenet reaches out and educates or punishes these companies. If you feel like this is moving too slowly or is too kind, we could investigate whether we can expand this team and perhaps take more decisive action sooner.
Finally, and this has been suggested before, we should either get rid of or limit access to the generic issue queue. Employees of these agencies have already admitted to scouring said issue queue for any module and picking up issues as they are at the top of the list. We should encourage people to mostly (or only) contribute to modules they are actively using.
Have you actually configured Group Sites? because it sounds like you're using the "Group from URL" context, which the module specifically advises you not to.
Proof of concept posted, needs tests and we need a follow-up issue to:
- Delete all references when a group role is deleted
- Create a UI element for reference deletion so we can circumvent the constraint
The fact that this throws an exception is good, otherwise it would have flown under the radar and caused security issues. Flexible Permissions (and therefore Access policy API) is right to crash loudly here, it's Group that is abusing the system by allowing us to change scopes without running a safety check first.
This needs to be fixed in Group.
Okay so I see two fixes:
- We run a script when role scope changes from individual to anything else to remove this role from all memberships
- We scan for this role on memberships and reject the save through a validation constraint
1. is destructive and potentially bad, 2. is annoying if you actually want 1. to happen.
Then there's door number three where we allow bad roles to be assigned, but ignore them in the calculator. But I'm really not a fan of allowing your data to be inconsistent with your code, so I'm very unlikely of accepting any such changes.
Currently erring on the side of 2, but maybe it's safer to do both. Write a service that mass revokes a role, then disallow the role change until you use said service to revoke the role. We could then use the same service to mass revoke roles when they get deleted.
@vensires I narrowed it down to the fact that changing a group role from individual to insider/outsider does not remove said role from existing memberships' "Group roles" field. That's what is causing the error. I'll see if I can come up with a sensible way of fixing this.
Thanks for the detailed report, now I have something to investigate (changing of scope on existing role)
Sorry for the noise, colors changed after I updated. Seems we're down to 6 now!
Whoops, removed wrong one. Rectifying.
Seems like this was fixed in 📌 Remove deprecated code from user module Fixed
10.3 has shipped already and it feels a bit odd to patch in a return type hint right before the release of 11. I would rather we drop the return type hint altogether and stick to the bare essentials: typehint the permission as a string and enforce this in 11.x
For that matter, we need to be quick about this too as 11.0 is to be release this week.
Pushed discussed change to the MR.
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:
- We already have that naming pattern for config
- 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.