Skip to content

Conversation

@michalsn
Copy link
Member

Description
This PR implements deep comparison for objects and arrays in Entity::hasChanged() and Entity::syncOriginal().

Previously, only shallow reference comparison was performed, meaning changes to object properties, array elements, or nested structures were not detected. This PR adds proper deep comparison using JSON normalization.

This may be considered a BC break, as existing applications that use objects within entities may now behave differently. For entities using only scalar types, there is no change in behavior.

Fixes #9777

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn michalsn added enhancement PRs that improve existing functionalities breaking change Pull requests that may break existing functionalities 4.7 labels Oct 30, 2025
@neznaika0
Copy link
Contributor

Nice. You can add JSON_UNESCAPED_UNICODE for readability? I'll test it later.

@neznaika0
Copy link
Contributor

  1. There are problems with other classes that do not extend JsonSerializable - DateTimeInterface, DateInterval, SplFileObject, ... At least the built-in Time should preferably be included in the list.
  2. You are not using __class, __data, __enum anywhere. Do I understand correctly that this is only for tracking that not only the value has changed, but also the object?

@neznaika0
Copy link
Contributor

  1. An additional option is to check the casting properties and convert it to raw format.
    $objectData = $this->castAs($data, $dbColumn, 'set')

@michalsn
Copy link
Member Author

  1. Good catch, I totally missed Time class... I don't know why, but I was sure it would be handled by JsonSerializable - fixed. In general, my thinking is that entities should contain DATA, not services/resources:
  • Timestamps, enums, nested entities, value objects are DATA
  • File handles, closures, DB connections, DOM objects are NOT DATA
  1. Yes, tracking changes only, nothing else.
  2. I don't think I understand the idea.

@neznaika0
Copy link
Contributor

I understand you, you stick to simplicity in many ways. I'm trying to expand possibilities by creating more complex solutions. It's not just about working with primitives. This is a better approach for a larger project than CRUD.

  1. I agree, many objects uses as services.
    I can't think of exact examples right now. Doctrine collection? In theory, it's a list, and it can be stored as an array in $original.

Casting converts it to primitives, so we can try using it in normalizeValue(). We will get values for json.

A small clarification: is it normal to lose microseconds? I never work with them, so I'm just thinking ahead. https://www.php.net/manual/en/datetime.constants.php#constant.date-rfc3339-extended

@michalsn
Copy link
Member Author

michalsn commented Oct 31, 2025

Good catch on the microseconds - fixed.

I've also added support for native SPL iterators. Modern collection implementations already provide a toArray() method.

I don't believe casting is the right approach in this case, since we don't require casting to be explicitly defined.

@neznaika0
Copy link
Contributor

Sad. I think casting is similar to toArray() or JsonSerializable. Because we get the object values.

Another case. Value Object (Phone, Address, Uuid...). They may not have an interface, but casting in CI4 for saving in the Model. or have the __toString() method.

@michalsn
Copy link
Member Author

michalsn commented Nov 1, 2025

Casting in our Entity implementation should be used only when seeding it with data containing scalar values. In other cases, casting should not be used.

Good call on __toString() - I added it as a fallback, when no public methods are available.

@neznaika0
Copy link
Contributor

Thanks, @michalsn for the huge PR. I don't have any questions right now. You need to use it to gain experience.

For VO, i will have to add an interface for compatibility. I think this is not a bad option.

Just to remind you, we have "nested relations" PR 😄 Maybe now it will be possible to combine it after the release of 4.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.7 breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants