Fix Block::load() N+1 and cache manifest generation#832
Conversation
Replace the per-block Block::load() call in dxpr_theme_preprocess_block() with the block entity already available in the render array via $variables['elements']['#block'], eliminating one entity load per block per page request. Guard _dxpr_theme_generate_manifest() and the apple touch icon entity loads (Media::load, ImageStyle::load) with file-existence checks so they only run when the cached files do not exist. Delete the cached files when theme settings are saved so they regenerate with updated values.
jjroelofs
left a comment
jjroelofs
left a comment
There was a problem hiding this comment.
Review findings from a local pass. GitHub will not allow this account to request changes on its own PR, but these two items should be treated as blocking before merge.
Validation performed locally:
git diff --check origin/8.x...origin/pr/832php -lagainstdxpr_theme.themefrom the PR headphp -lagainsttheme-settings.phpfrom the PR head
I could not run PHPCS because this checkout does not have vendor/bin/phpcs installed.
| function dxpr_theme_preprocess_block(&$variables) { | ||
| if (isset($variables['elements']['#id'])) { | ||
| $block = Block::load($variables['elements']['#id']); | ||
| $block = $variables['elements']['#block'] ?? NULL; |
There was a problem hiding this comment.
This looks like it will silently skip the existing block behavior. Drupal core adds #block for BlockViewBuilder::preRender(), but that same pre-render callback unsets #block before the block theme hook and theme preprocess callbacks run. By the time dxpr_theme_preprocess_block() runs, $variables['elements']['#block'] is normally gone, so $block stays NULL and we stop setting region and the block_classes content_class. The block templates use region for the card/sticker logic, so this is a visible regression. We need another way to carry those values before removing the Block::load() fallback.
|
|
||
| _dxpr_theme_generate_manifest($web_icons_media_id, $manifest_path); | ||
| // Generate manifest only when the file does not already exist. | ||
| if (!file_exists($manifest_path)) { |
There was a problem hiding this comment.
Caching the manifest solely by file existence makes it stale for inputs that are not invalidated by the theme-settings submit handler. _dxpr_theme_generate_manifest() reads system.site:name, so changing the site name or importing config that changes system.site will keep serving the old manifest name/short_name until someone happens to save theme settings. Please key or regenerate the cached manifest from all of its inputs, or invalidate it when those inputs change.
BlockViewBuilder::preRender() unsets #block before theme preprocess runs, so $variables['elements']['#block'] is always NULL. Restore Block::load() which uses the entity static cache. Regenerate manifest when the site name changes instead of only when the file is missing, so config changes to system.site are reflected.
1 participant
Linked issues
Solution
Two performance fixes:
Block::load() N+1: Replace the per-block
Block::load()call indxpr_theme_preprocess_block()with the block entity already available in the render array ($variables['elements']['#block']), eliminating one entity load per block per page.Manifest regeneration: Guard
_dxpr_theme_generate_manifest()and the apple touch icon entity loads (Media::load,ImageStyle::load) with file-existence checks so the manifest and icon URLs are only computed when the cached files do not exist. Delete the cached files when theme settings are saved so they regenerate with updated values.Checklist