tribe-common icon indicating copy to clipboard operation
tribe-common copied to clipboard

Handle model caching correctly

Open lucatume opened this issue 1 year ago • 0 comments

Ticket: TEC-4379

Artifacts: tests and screencast

This commit updates the logic used in the Post decoration process to make sure it plays nice with different caching solutions.

Depending on the specific caching solution or plugin, the failure will come down to PHP trying to build, during an unserialize call, objects of not yet defined classes. This might happen for 2 main reasons:

  1. When the object was cached plugins A, B and C were active; if only A is active when the object is unserialized, then this will lead to a cascading failure when PHP tries to build objects of classes defined by the plugins B and C.
  2. The object is unserialized before the plugins defininig the classes are loaded, the result will be, as is the case for 1, a cascading failure during unserialization.

Unserialization failures will trigger, by default, a warning (E_WARNING) that will not stop the code execution and will propagate incoherent of partially built values down the execution line.

The main cause of this kind of issues would be posts cached in the posts group. Functions like tribe_get_event will decorate these post objects adding dynamic properties to them (e.g., is_now or start_date or venues). Any one of these properties that is not a scalar or an instance of an object defined by Core PHP (e.g. stdClass or DateTime) will likely cause this failure.

The posts cache group, in particular, is consistenly pre-fetched by many caching solutions when trying to push performance, it's beyond TEC control.

Given TEC code, and other developer codes, depends on the dynamic properties mentioned above, we cannot remove them. Also, we cannot control the serialization of a WP_Post instance as it is unfiltered internal code. With these premises the solution is two-fold:

  1. Make sure no property dynamically added to WP_Post instances is an object of a class not defined by PHP Core.
  2. Alleviate the cost of re-decorating each WP_Post instance by caching the proerties in a way under our control (a _tec_pre_serialized property is added to the WP_Post)
  3. Memoize the decorated WP_Post instance, do not cache it.

PHP 8.2 will deprecate dynamic properties, i.e., all the ones we decorate the WP_Post instance with. The WP_Post class will look for undefined properties in the meta (see WP_Post::__get), but meta lookup is really inefficient since the value will be filtered each time it's fetched. I've thought about using this refactoring to mitigate that problem too, but the Core team is steering toward the wide-spread use of the AllowDynamicProperties attribute; I've hence decided not to move dynamic properties to meta.

lucatume avatar Oct 25 '22 13:10 lucatume