oroinc / platform

Main OroPlatform package with core functionality.
Other
627 stars 351 forks source link

User Object in Config breaks ExtendEntity cache by loading ExtendUser before EntityExtend replaces it #1059

Open DylanSale opened 3 years ago

DylanSale commented 3 years ago

Summary
Adding a field with type User object (or any object that is Entity Extend compatible and has fields added) in the Config table causes the oro:entity-extend:config:warmup command to throw a RuntimeReflectionService exception saying Property xxx does not exist.

This happens because the warmup command uses the Logger, which gets the detail log format from the ConfigManager which loads the Config from the database, including the User entity. The User then loads the ExtendUser class which can no longer be replaced by the EntityExtendBundle with the generated class.

Steps to reproduce
Install oro ~4.1.11 (though I think this happens in newer versions of 4.1 as well)

Ensure the User object has a custom extend entity field defined. This is true by default in orocrm.

Add a config field in the system_configuration.yml

    foo.task_default_user_commercial:
      data_type: scalar
      type: Oro\Bundle\UserBundle\Form\Type\UserSelectType

Set the config value to a user in the UI.

Run

 rm -rf var/cache
 bin/console c:c
 bin/console oro:entity-extend:config:warmup

Actual Result An Exception saying something like:

RuntimeReflectionService exception saying Property xxx does not exist.

Expected Result

It should finish normally.

Details about your environment

DylanSale commented 3 years ago

For now we are working around this by changing the config to use a scalar id and loading the entity in the code where it is used.

It would be better if Oro handled this somehow, or at least raised an error if an EntityExtend object is put in the Config

anyt commented 3 years ago

Storing an object in the system configuration is allowed, and you even can put there a doctrine entity. Still, in this case, you have to manually refresh the entity state after fetching it from the database, so we recommend storing there only scalar values like you actually did. For now, we don't see a way to prevent this issue by some checks in OroPlatform, as the entity may be stored in the config intentionally. So I guess we can close it.

DylanSale commented 3 years ago

If it is expected that users can store EntityExtend objects in the Config fields then I suggest Oro platform needs to handle this more gracefully as it currently breaks the EntityExtend functionality and the oro:platform:update command (which runs the entity extend cache warmup command) when this is done and the entity extend cache metadata will no longer be updated.

This took us about 2 days to debug and blocked production deployments during that time as it was not obvious that storing an entity extend object in the config was related to the error message and during that time the entity extend cache was broken so all functionality in the system that relied on the User entities was also broken.

Perhaps a potential fix is not loading ConfigManager during the entity-extend:cache:warm command? I saw that the localebundle has an explicit check for entity extend cache commands in its command listener (maybe a related issue?), perhaps there are more places these checks need to occur to stop loading the ConfigManager during the entity extend cache commands?

Thanks