Bug new values incorrect detection

[darealfive]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	✔️
Fixed issues	#522

Problem

AbstractActiveRecord::newValues() uses strict comparison (!==) also for DateTimeInterface attributes,
which causes identical date/time values to be marked as changed because they are different objects.

Solution

If the new value implement DateTimeInterface, compare both objects by comparing the formatted date Y-m-d\TH:i:s.uP, e.g.: 2011-02-03T04:05:06.123456+01:00.
This fixes false positives and works also when two different date time objects represents the same moment in time but have different time zones.
E.g.:

• 2011-01-01T01:01:01.111111+01:00
• 2011-01-01T00:01:01.111111+00:00

... they both represent the same moment in time, but the formatted string is clearly different and thus can be detected as changed by the newValues() function.

Tests

Added tests to cover:

• identical DateTime values not marked as dirty
• identical moment in time values marked as dirty, because of different time zone
• different DateTime values correctly detected as changes

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.93%. Comparing base (dbedcb3) to head (e138d9b).
⚠️ Report is 36 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@## master #526 +/- ##
=============================================
- Coverage 100.00% 99.93% -0.07% - Complexity 654 658 +4 
=============================================
Files 43 43 Lines 1632 1610 -22 =============================================
- Hits 1632 1609 -23 - Misses 0 1 +1 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[darealfive]

I only added test data (table customer) only for sqlite database driver. This obviously leads to failing tests...

@Tigrov or @vjik , would you add the fixtures for the remaining database drivers except sqlite?
I only managed to run the tests for sqlite.

[darealfive]

I managed to get running the tests with different DB drivers - it turns out I missed your makefile in this repo making running the tests with corresponding docker services a breeze :)

Unfortunately, I have to adjust the tests based on the DB drivers, because DATETIME columns are different depending on the DBMS and not always supports storing the timezone.

Give me some time to adjust the tests.

[Copilot]

Pull request overview

This PR fixes a bug in AbstractActiveRecord::newValues() where DateTimeInterface attributes were incorrectly marked as changed due to strict object comparison (!==). The fix compares DateTimeInterface objects by their formatted string representation (Y-m-d\TH:i:s.uP), which includes timezone information, preventing false positives while still detecting genuine changes including timezone differences.

Key changes:

• Modified newValues() method to use formatted string comparison for DateTimeInterface objects
• Added comprehensive test coverage for DateTime comparison scenarios
• Updated test fixtures with registered_at column and data to support DateTime testing

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/AbstractActiveRecord.php	Core fix: Added special handling for DateTimeInterface comparison using formatted strings instead of strict object comparison
tests/ActiveRecordTest.php	Added test provider and test method covering new records, unchanged DateTime values, and timezone-different DateTime values
tests/ActiveQueryTest.php	Added tests verifying newValues() behavior with same/different moments in time; updated existing test expectations
tests/ActiveQueryFindTest.php	Updated test expectations to include the new registered_at field
tests/ArrayableTraitTest.php	Updated test expectations to include the new registered_at field in toArray() output
tests/Stubs/ActiveRecord/Customer.php	Added registered_at property and getter/setter; implemented fields() method to format DateTime for array serialization
tests/Stubs/ActiveRecord/CustomerClosureField.php	Added registered_at property and field formatter in fields() method
tests/data/sqlite.sql	Added registered_at column to customer table and populated test data with DateTime values including timezone information

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When the new value is a DateTimeInterface, the code checks if the old value is null but doesn't verify that the old value also implements DateTimeInterface before calling format() on it at line 130. If the old value is a non-null value that doesn't implement DateTimeInterface (e.g., a string), this will cause a fatal error. Consider adding an additional check: || !($oldValues[$name] instanceof DateTimeInterface) to the condition at line 129.

Suggested change

	if ($oldValues[$name] === null
	if ($oldValues[$name] === null
	|| !($oldValues[$name] instanceof DateTimeInterface)

[Tigrov]

Also see what Copilot suggests

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4fb3e91c-5f8e-4034-a16d-08bf0a40e471

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[darealfive]

@Tigrov Given my limited time, I’ve done everything I could to fix the bug. The only thing I skipped were the Oracle DB tests, as I couldn’t figure out how to do them.

A friend of mine is going to take over from here :)
Apologies for the delay.
Best regards, Sebastian