Problem/Motivation

#3566626: Use property hooks for entity reference item and list entity and target_id properties turned $entity and $target_id on EntityReferenceItem and EntityReferenceFieldItemList into PHP property hooks. These are virtual properties: they have no stored value.

DependencySerializationTrait::__sleep() returns array_keys(get_object_vars($this)). Since PHP 8.4, get_object_vars() includes virtual properties. So __sleep() returns entity and target_id, and serialize() then warns:

Warning: serialize(): "entity" returned as member variable from __sleep() but does not exist

.

Steps to reproduce

$items = \Drupal\user\Entity\User::load(1)->get('roles');
serialize($items);

Will result in the warning for both entity and target_id.

Proposed resolution

Leave virtual properties out in DependencySerializationTrait::__sleep(). Their values come from other properties, so nothing is lost. Add a kernel test that serializes an entity reference field item and its list, and checks that there is no warning.

Remaining tasks

  • Fix and test.

Generated with the help of an LLM.

Reviewed by a human though.

Issue fork drupal-3634852

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

gábor hojtsy created an issue. See original summary.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize()

probably related to #3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize()

gábor hojtsy opened merge request !17657

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs review

Submitted an MR. DependencySerializationTrait::__sleep() used get_object_vars(), which lists virtual properties and runs their get hooks. That is why serialize() then warned that the property does not exist. If we read the stored values with an array cast instead, that leaves virtual properties out and runs no hooks, so no warning :)

godotislate’s picture

godotislate
he/him
commented

Seems like we might be able to use get_mangled_object_vars instead of get_object_vars: https://stackoverflow.com/a/79410342, but that could cause BC issues with already serialized objects because the key names would be different. (Using get_mangled_object_vars also seems like it might make serializing private properties possible?). Or we could normalize the keys from get_mangled_object_vars.

An alternative would be to do reflection while looping through get_object_vars, and check if the property isVirtual.

Also per #4, switching to using __serialize/__unserialize might make this kind of problem go away, (though maybe not if the virtual property does not allow set) but it probably makes sense to filter out hooked properties regardless. And we can't get #3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize() in before 12.1.

godotislate’s picture

godotislate
he/him
commented
Title: Serializing an entity reference field item fails since it uses property hooks » DependencySerializationTrait should ignore hooked properties
Status: Needs review » Needs work

MR is failing PHPStan:

------ ----------------------------------------------------------------------- 
  Line   core/lib/Drupal/Core/DependencyInjection/DependencySerializationTrait  
         .php (in context of class                                              
         Drupal\Tests\Core\DependencyInjection\DependencySerializationVirtualP  
         ropertyTestDummy)                                                      
 ------ ----------------------------------------------------------------------- 
  105    Storing entity storage as a class property is not recommended. Call    
         Drupal\Core\Entity\EntityTypeManagerInterface::getStorage() at the     
         call-site instead.                                                     
         🪪  drupal.entityStoragePropertyAssignment                             
         💡  See                                                                
         https://mglaman.dev/blog/dependency-injection-anti-patterns-drupal     
 ------ ----------------------------------------------------------------------- 
 ------ ----------------------------------------------------------------------- 
  Line   core/tests/Drupal/Tests/Core/DependencyInjection/DependencySerializat  
         ionVirtualPropertyTestDummy.php                                        
 ------ ----------------------------------------------------------------------- 
  31     Drupal\Core\DependencyInjection\DependencySerializationTrait does not  
         support private properties.                                            
         🪪  dependencySerializationTraitProperty.unsupportedPrivateProperty    
         💡  See https://www.drupal.org/node/3110266                            
 ------ -----------------------------------------------------------------------

The entity storage thing should just be added to baseline.

The private property thing probably needs looking into. Though I touched on it in #7, we probably shouldn't add support for them here. #3110266: Support serialization of private properties is the issue for that.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Needs review

Thanks! I am looking into get_mangled_object_vars(). It looks like it gives the same result as the array cast. No virtual properties, and no get hooks. It is also clearer, and safer for ArrayObject subclasses. The keys are turned back into plain property names, so serialized data stays the same.

If we would loop over get_object_vars() with isVirtual() that would run every get hook first. For entity reference items, that loads the referenced entity during serialization.

Agreed we should not add private property support here and that #3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize() is the better long-term fix for serialization. But property hooks are already in 12.0, so this is needed until then.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Title: DependencySerializationTrait should ignore hooked properties » Drupal 12 regression: DependencySerializationTrait should ignore hooked properties