yiisoft / yiisoft/active-record

`AbstractActiveRecord::newValues()` incorrectly marks DateTimeInterface attribute as changed

Open Beginner friendly
#522 11 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

type:bug
Dominant language
PHP
Stars
119
Forks
38
Avg merge
1h 11m
Merged PRs (30d)
1

Description

Description

The newValues() method fails to correctly detect changes for DateTimeType related types, e.g. DateTimeInterface respectively DateTimeImmutable.

Specifically:

  • DateTimeImmutable / DateTimeInterface values are compared using strict comparison (!==), causing two instances representing the same moment to be always treated as changed (incorrectly), because two objects are never object-equal.

This affects Active Record dirty-checking and can cause unnecessary record updates and therefore subsequent problems.


Steps to Reproduce
  1. Load an Active Record model from the database with at least one DateTime column and make sure it gets mapped to DateTimeImmutable.
  2. ...either assign a new DateTimeImmutable instance with the same date/time value.
    ... or leave the value as is.
  3. Call newValues() or attempt to save the model.Expected:
    The attribute should not be marked as dirty.

Actual:
The attribute is considered as changed (dirty) incorrectly.


Expected Behavior
  • DateTimeInterface values should be compared using loose comparison (==) to compare actual date/time values rather than object identity.

Proposed Fix
/**
 * Returns the property values that have been modified since they're loaded or saved most recently.
 *
 * The comparison of new DateTimeInterface objects with old values uses `==`.
 * The comparison of all other values uses `===`.
 *
 * @param array|null $propertyNames The names of the properties whose values may be returned if they're changed
 *                                  recently. If `null`, {@see propertyNames()} will be used.
 *
 * @return array The changed property values (name-value pairs).
 *
 * @psalm-return array<string, mixed>
 */
#[Override]
public function newValues(array|null $propertyNames = null): array
{
    $values    = $this->propertyValues($propertyNames);
    $oldValues = $this->oldValues();
    if ($oldValues === []) {
        return $values;
    }

    $result = array_diff_key($values, $oldValues);
    foreach (array_diff_key($values, $result) as $name => $value) {
        if ($value instanceof DateTimeInterface) {
            if ($value != $oldValues[$name]) {
                $result[$name] = $value;
            }
        } else {
            if ($value !== $oldValues[$name]) {
                $result[$name] = $value;
            }
        }
    }

    return $result;
}

Notes

I think this behavior avoids false positives when using immutable date/time objects.
You may update \Yiisoft\ActiveRecord\ActiveRecordInterface::newValues documentation with the provided one and leave the actual implementation documentation empty (as it is currently).

Package version

1.0.0

PHP version

PHP 8.4.15 (cli)

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with newValues() in src/AbstractActiveRecord.php and compare its current DateTimeInterface handling with the issue's reproduction. Check the corresponding newValues documentation in Yiisoft\ActiveRecord\ActiveRecordInterface. Done means equivalent DateTimeImmutable values are not reported as dirty while other values retain strict comparison behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
databases
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
65/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.