Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
JoaoSetas opened this issue Jan 4, 2024 · 3 comments
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium

Comments

@JoaoSetas
Copy link

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

@JoaoSetas JoaoSetas added bug A bug report status: unverified Unverified labels Jan 4, 2024
@JoaoSetas JoaoSetas changed the title [BUG]: Bug with recent change for toArray() to call getters. PR #16469 [BUG]: Bug with recent change on toArray() to call model getters. PR #16469 Jan 4, 2024
@rudiservo
Copy link
Contributor

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
Copy link
Contributor

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

@JoaoSetas JoaoSetas changed the title [BUG]: Bug with recent change on toArray() to call model getters. PR #16469 [BUG]: Recent changes on toArray() breaks caching and serialization. PR #16469 Jan 4, 2024
@niden niden added status: medium Medium 5.0 The issues we want to solve in the 5.0 release and removed status: unverified Unverified labels Jan 4, 2024
@niden
Copy link
Sponsor Member

niden commented Jan 4, 2024

Resolved in #16491

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

@niden niden closed this as completed Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 The issues we want to solve in the 5.0 release bug A bug report status: medium Medium
Projects
Status: Released
Development

No branches or pull requests

3 participants