Refactor storage class generation

FWIW, I added some `sleep()` calls in that code, executed drush cr and drush updb -y from the CLI, then browsed the site while the commands were still running, and I didn't bump into that error.

However, if you find that the MR makes a difference in your case, I've no problem with merging it :)

@socialnicheguru, there's nothing to review here, the patch in #5 is just meant for debugging, and the MR doesn't contain any commits (anymore).

Did you bump into this problem as well?

Sorry I was looking at this link and thought there was something to review: https://git.drupalcode.org/project/trash/-/merge_requests/25/diffs?commi...

I think that commit only reverts the initial commit from the MR.

I'm going to close this issue. @DieterHolvoet, please feel free to re-open it if you managed to get more info about this.

drush updb can cause it too. I'm getting it on ~25 of 100 sites when running that command

2026-01-15T15:08:33.1728048Z   Unable to decode output into JSON: Syntax error                              
2026-01-15T15:08:33.1729094Z                                                                                
2026-01-15T15:08:33.1730175Z   Fatal error: Cannot redeclare class Drupal__node__NodeStorageTrash696902ec8  
2026-01-15T15:08:33.1731416Z   a9fe (previously declared in /files/php/trash/Drupal__node__NodeStorageTras  
2026-01-15T15:08:33.1732680Z   h696902ec8a9fe/SWGc4J0Q_JFEySFJ8kZuyJFBKDNDHy1jTGfvzzLX2Oc.php:8) in /files  
2026-01-15T15:08:33.1733954Z   /php/trash/Drupal__node__NodeStorageTrash696902ec8a9fe/NAgMAKMfBvCxcPrxUxdg  
2026-01-15T15:08:33.1735220Z   0qvzCj9E2Vdw5h_nNTE6jLw.php on line 8                                        

Started a new MR with an approach I wanted to try for a long time. It's quite a big change, but it should be much more reliable than the current way we swap storage handlers.

I ran into this same issue during a updatedb as well.

Unable to decode output into JSON: Syntax error
Fatal error: Cannot declare class Drupal__node__NodeStorageTrash69453109e6dee, because the name is already in use in /files/php/trash/Drupal__node__NodeStorageTrash69453109e6dee/J6qrJa6NuNd6zClRjR1f2_YxhXXcdkgwH5AFRDiXjn8.php on line 8

It seemed to happen after a module ran a cache-clear or rebuilt the NodeStorage as a part of its update hook

I developed a workaround to add to the deployment steps, which was to run:

drush php:eval "\Drupal::service('file_system')->deleteRecursive('sites/default/files/php/trash');"')

I tried to debug locally and struggled, so I asked AI for help, this is how it described the problem:

Root Cause                                                                                                                                                                                                                 
The MTimeProtectedFileStorage generates file paths that include a hash based on the directory's mtime (line 135 in MTimeProtectedFastFileStorage.php):

return $directory . '/' . Crypt::hmacBase64($name, $this->secret . $directory_mtime) . '.php';                                                                                                                             

The Problem Flow:                                                                                                                                                                                                          

1. Bootstrap: File exists at path files/php/trash/ClassName/HASH_A.php → include_once loads class                                                                                                                          
2. ConfigEntityUpdater triggers entity rebuild                                                                                                                                                                             
3. useCaches(FALSE) or clearCachedDefinitions() deletes the PHP storage files                                                                                                                                              
4. Storage requested again → _trash_generate_storage_class() called                                                                                                                                                        
5. New suffix generated (or same suffix - doesn't matter)                                                                                                                                                                  
6. File doesn't exist → regenerated with new directory mtime → stored at files/php/trash/ClassName/HASH_B.php                                                                                                              
7. $php_storage->load() calls include_once on HASH_B.php                                                                                                                                                                   
8. PHP sees this as a different file (different path!) and includes it                                                                                                                                                     
9. Class declaration executes → FATAL: Class already in use                                                                                                                                                                

The include_once only prevents re-including the same file path, not the same class name.                                                                                                                                   

The Fix                                                                                                                                                                                                                    
In trash.module line 229, add a class_exists() check before calling load():                                                                                                                                                
// Current code (buggy):                                                                                                                                                                                                   
if ($php_storage->load($trash_class . '.php')) {                                                                                                                                                                           
  return $trash_class;                                                                                                                                                                                                     
}                                                                                                                                                                                                                          
// Fixed code:                                                                                                                                                                                                             
if (class_exists($trash_class) || $php_storage->load($trash_class . '.php')) {                                                                                                                                             
  return $trash_class;                                                                                                                                                                                                     
}                                                                                                                                                                                                                          

This prevents attempting to redeclare a class that's already loaded in PHP memory.                                                                                                                                         

My issue with testing the solution is that the issue seems intermittent, so I can't determine if I am lucky and the update hasn't broken it, or if the fix suggested is working.

After splitting out #3569052: Move TrashStorageTrait::buildQuery to a hook implementation and #3569155: Refactor TrashStorageTrait::setPersistentCache to handle the protected methods we were overriding from SqlContentEntityStorage, the refactoring MR is green!

Even though I trust the test coverage that we currently have, I'd appreciate any real-world testing for it :)

The comment on MemoryBackend::reset() says it's only called during tests, but this is false. If you go to /admin/reports/status it gets called via EntityDefinitionUpdateManager when it generates its changelist.
It just assumes the method is there. :(
(Related issue discussing not using MemoryBackend as it's designed for tests: #2973286: Clean up MemoryCache and MemoryBackend naming issues.)

After fixing that I stumbled upon a more fundamental problem with this approach. A general decorator is neat, but it can't easily implement all the expected interfaces for all storages. Just going to /node/1/revisions shows NodeController::getRevisionIds() expects NodeStorageInterface but got the TrashEntityStorageDecorator instead.

I'm not sure how we could get around that without generating adapter classes.

Woah.. it's kinda embarrassing that I missed that aspect :)

Ok, so a general decorator won't work, but friendsofphp/proxy-manager-lts seems to do the job quite well!

Patch from MR:

@terry_kolodiy, that patch looks like an old revision of the MR, which had various problems. You should try to use the latest code from both 3.1.x and the MR.

Even though I liked the "cleanliness" of the proxy manager approach, I equally dislike bringing in a big dependency like that.

So here's a new MR which just refactors the storage class generation to hopefully handle all possible cache clearing problems and edge cases around multiple web heads.

Merged into 3.1.x!

Just had another update hook do a cache clear during a deployment and got caught with this.
Does this need to be backported to 3.x? If I update to the latest stable (3.0.27), will this be solved?
For some reason, I am still on 3.0.18. I'll look into why violinist isn't doing its thing.

@the_g_bomb, there were a few other changes related to this refactor that were done outside this issue, and it would be hard to track down each of them.

But you can safely update to 3.1.x-dev though if you're on Drupal 11.3. This work is the only major change between the 3.x and 3.1.x branches at the moment, so that branch is not in an "unstable" state by any means :) It will start getting official releases when #3376216: Translation support gets done.