phalcon / cphalcon

High performance, full-stack PHP framework delivered as a C extension.
https://phalcon.io
BSD 3-Clause "New" or "Revised" License
10.79k stars 1.96k forks source link

[BUG]: Recent changes on toArray() breaks caching and serialization. PR #16469 #16490

Closed JoaoSetas closed 10 months ago

JoaoSetas commented 10 months ago

Description: We have encountered significant backward-compatibility issues with the latest update of Phalcon concerning the behavior of the toArray() method on models. These changes have introduced two critical problems that are disrupting our projects.

Issue 1: Unexpected Behavior Change in toArray() Method

Our projects heavily rely on the toArray() method to return the model's original properties without invoking the getters. As of the recent changes, toArray() now considers the model's getters, leading to unexpected return values that diverge from our application logic.

To illustrate, consider a model with the following getter:

public function getParams($as_array = true)
{
    return json_decode($this->params, $as_array);
}

Previously, invoking toArray() on this model would yield a string value "{}". After the update, it now returns an array [], causing code to break where a string was expected. To address this, we propose introducing a configuration option that allows us to preserve the original behavior of toArray(), facilitating a smoother transition in upgrading projects without demanding immediate, extensive refactoring.

Issue 2: Alteration in Model State After Unserialization Due to toArray() Use in Serialization

The second, and possibly more concerning issue, is related to model serialization and caching. It appears that the serialization process inadvertently triggers toArray(). Consequently, when we unserialize or retrieve a model from the cache, it can present a state that is fundamentally different from when it was serialized.

This behavior is problematic because it violates the principle that serialized and subsequently unserialized entities should remain consistent. The unexpected mutation of model states during serialization undermines the stability of our applications and disrupts data integrity.

We would like to see a resolution to this issue that ensures models retain their original state post-unserialization, thereby preventing these unintended and disruptive changes.

By addressing these two issues, we can ensure that applications depending on the Phalcon framework can continue to operate reliably and that developers can upgrade to the latest version with minimized friction.

Details

Phalcon version: 5.5.0 PHP Version: 8.1.27 Operating System: Docker image - php:8.1-fpm Installation type: installing via package manager Server: Nginx Database: mysql:8

rudiservo commented 10 months ago

Use of getters for any other behaviour other than returning the value of the property has a usable value is not common practice. Phalcon has been using this behaviour in the Forms for a very very long time, the change in toArray() to try and fetch the getter value only makes it a more consistent behaviour, not a breaking.

Although would I agree that it could be optional, you can easily override the toArray() function with your own.

rudiservo commented 10 months ago

@niden, issue is with serialize using toArray, the function needs and extra parameter not to use the getter.

niden commented 10 months ago

Resolved in https://github.com/phalcon/cphalcon/pull/16491

Thank you @JoaoSetas for reporting and @rudiservo for fixing!