The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇦🇺Australia acbramley
This needs a reroll
Also
drupal_static
is being deprecated (see #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() → ) so we should not add new usages of it. This would require moving node_access_grants into a service and deprecating the function. - 🇬🇧United Kingdom catch
This is still overall a scalability issue in Drupal core, and there are some contrib approaches these days like https://www.drupal.org/project/entity_hierarchy → which uses nested sets.
Moving back to 'active' but converting to a plan.
- 🇺🇸United States smustgrave
Thank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. 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!
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Very nice to see we’ve come so far that we can deprecate that! 🥳
- 🇬🇧United Kingdom catch
I'm also seeing these in 'chained http requests' in Lighthouse with stock Drupal 11.x and the standard profile, so I don't think it's Drupal CMS specific.
…lora/lora-v14-latin-italic.woff2 …lora/lora-v14-latin-700.woff2
Changing this to a bug report because Olivero is implementing preloading but it's currently failing to preload what it needs.
The italic font appears to be loaded purely for the
You haven’t created any frontpage content yet.
although that wouldn't apply to Drupal CMS, but it could be two different reasons for the same thing. Couldn't yet figure out what's causing the 700 weight font to be loaded. - 🇬🇧United Kingdom catch
I'm doing this in 📌 Refactor system/base library Needs work
- First commit to issue fork.
- 🇨🇭Switzerland berdir Switzerland
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.
- @berdir opened merge request.
- 🇬🇧United Kingdom catch
@gapple if hook_rebuild() is being used for pure cache pre-warming (e.g. something that will still be populated on a cache miss) then yes it could switch to this.
The differences will be the drush parallel cache warming, and if there's a stampede, the ability to warm different caches at random during lock wait or while waiting for async database queries to come back. Whereas hook_rebuild() has to go linerlarly through whatever is put in it.
None of these are implemented yet but see related issues.
- 🇨🇦Canada gapple
Would prewarming eventually be a replacement for
hook_rebuild()
when it's no longer@internal
, or will they have different use cases? - 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Oh wow, for a second there my brain processed the assert as an if-statement. Left one more suggestion, but that can be fixed upon commit.
- 🇺🇸United States smustgrave
Following the steps in the issue summary confirmed everything is behaving as normally.
- 🇬🇧United Kingdom catch
If CachedStrategy does not find any hits, it will also return an empty array. Isn't that the same as what the test strategy did?
The only difference I see is that the test did not have SingleFlushStrategy as a fallback, which ends up returning all placeholders anyway.Yes exactly - SingleFlushStrategy always replaces the remaining placeholders, which is why we can do the assert() to make sure there's none left to replace.
The unit test has no fallback, just a no-op strategy, and in that situation the 'removed' placeholder is not actually removed, but just ignored and not replaced, which we are now validating should never be allowed to happen.
So does that not mean that the check has to be !empty() to figure out that something was, in fact, not replaced?
No because the failure condition for an assert() call is when it returns FALSE.
https://www.php.net/manual/en/function.assert.php
Assertions should be used as a debugging feature only. One use case for them is to act as sanity-checks for preconditions that should always be true...
- 🇨🇭Switzerland berdir Switzerland
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.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
It is not possible to do this, you can ignore placeholders, you can't remove them.
If CachedStrategy does not find any hits, it will also return an empty array. Isn't that the same as what the test strategy did? BigPipeStrategy can also return an empty array. The only difference I see is that the test did not have SingleFlushStrategy as a fallback, which ends up returning all placeholders anyway.
Might be my sickness talking but if I read ChainedPlaceholderStrategy::processPlaceholders() correctly, we reduce the set of $placeholders whenever a strategy returned some of them back. At the end of the loop we want $placeholders to be empty, no? So does that not mean that the check has to be !empty() to figure out that something was, in fact, not replaced?
- 🇬🇧United Kingdom catch
Ideally, we don't go to the cache at all, which would be possible using a tag like CACHE_NOT_WORTH_IT or RENDER_CACHE_REQUIRED.
We use
'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form'
to prevent caching of forms on POST requests now, so adding another 'special' cache tag would be consistent with that.I think manual intervention is fine: we're mainly interested in:
1. Very high cardinality cache items that are usually inside something else, mostly to save writes/space. Like the paragraphs example but that already has a separate opt-out.
2. Very high frequency cache items that are as-cheap to render without caching, mostly to avoid unnecessary gets, like some of the 'site chrome' blocks identified by @berdir.
- 🇬🇧United Kingdom catch
Render cache multiple get is now in core after 📌 Render the navigation toolbar in a placeholder Active , we should see if there's an example in core where this would help, and then try to implement it in the renderer.
- 🇬🇧United Kingdom catch
That makes sense, just missed it, sorry for the false alarm.
Committed/pushed to 11.x, thanks!
- 🇬🇧United Kingdom catch
I think because of a small mistake.
I thought I'd missed the exclamation mark when you pointed this out, but it was actually fine, the problem was in the test.
The unit test sets up a 'placeholder removing strategy' that returns an empty array. It is not possible to do this, you can ignore placeholders, you can't remove them. So I just removed this from the data provider, it's testing something that's not supported and pretty sure the unit test would have choked on the new assertion since it was added.
- 🇺🇸United States smustgrave
Lets try and get to this before the bot attacks again :)
Actually all feedback appears to be addressed.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
ChainedPlaceholderStrategyTest is choking on the new assert, I think because of a small mistake.
Americanizzze the code comments.
😂
- 🇬🇧United Kingdom catch
Think I got to all the feedback, agreed on the more general assertion message.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Dug up the ::class docs for people stumbling upon this issue: https://www.php.net/manual/en/language.oop5.basic.php#language.oop5.basi...
Found some. minor things but looks good to me otherwise
- 🇨🇭Switzerland berdir Switzerland
Rebased and squashed the commits together to make future rebases easier.
- 🇳🇿New Zealand quietone
Asked in slack about how to remember to add the type hint in 12.0.0 when the trigger_error is deleted. @larowlan suggested a new issue. The issue is 📌 Add type hint to RefinableCacheableDependencyTrait::addCacheableDependency Postponed .
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇺🇸United States smustgrave
Just following up on this, if still a valid feature could the summary maybe be updated?
If no follow up this could be eligible for closure in 3 months
Thanks all!
- 🇺🇦Ukraine nginex
don't see any blocker why not to add UnchangingCacheableDependencyTrait, so views can still have max-age -1
If it breaks things, please report it here
- Issue created by @darko_antunovic
- 🇦🇺Australia acbramley
We need a title and IS update here about exactly what we're proposing to change.