Problem/Motivation

Both the Image and the Icon Text components promote their referenced file to permanent and then save the File entity, unconditionally, inside build(). There is no isTemporary() guard:

$image = $this->entityTypeManager->getStorage('file')->load($fid);
$image->setPermanent();
$image->save();

The intent is sound: a file that has just been uploaded through the managed_file element is temporary, and if it is never marked permanent Drupal's file garbage collection will delete it. But that promotion only needs to happen once, when the file first becomes referenced by the block. Doing it inside build() means a full entity save - a database write, plus cache tag invalidation for that file - happens on every render of the component: every uncached page view, every cache rebuild, every layout preview.

The second and subsequent saves are pure waste. The file is already permanent, so the save writes the same values back, and it does so on the critical rendering path.

Steps to reproduce

  1. Add an Image (or Icon Text) component to a layout and upload an image.
  2. Save the layout and view the page so the component renders once - the file is now permanent.
  3. Clear caches and view the page again.
  4. Observe that File::save() runs again even though the file status is already permanent. Any query log, or a breakpoint on File::preSave(), shows the write.

Proposed resolution

Guard the promotion so it only runs when the file is actually temporary:

if ($image->isTemporary()) {
  $image->setPermanent();
  $image->save();
}

This preserves the existing behaviour for freshly uploaded files and eliminates the write on every other render.

Affects both branches and both components. Confirmed line references:

  • src/Plugin/Block/LBKImage.php - 4.0.x lines 238-241, 3.0.x lines 232-235
  • src/Plugin/Block/LBKIconText.php - 4.0.x lines 225-228, 3.0.x lines 232-235

Credit to ecj: this guard is present in their patch on #3318209. It sat there unreported, bundled inside a feature request, so the defect was never filed on its own. Splitting it out here so it can be fixed independently of the feature work.

Remaining tasks

  • Apply the isTemporary() guard to both components on both the 4.0.x and 3.0.x branches.
  • Decide, separately, whether saving entities inside build() is appropriate at all. Render callbacks should not have side effects; the natural home for the promotion is blockSubmit(), where the file reference is established. That is a larger behavioural change - it needs an upgrade path for blocks whose files were never rendered - so it deserves its own issue rather than being folded into this fix.
  • Add Kernel test coverage asserting that rendering a component whose file is already permanent performs no File entity save.

Comments

aangel created an issue.