Skip to content

feat!: use mongodb function to check for dirtiness#2515

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

Open
wants to merge 2 commits into
base:5.x
Choose a base branch
from

Conversation

divine
Copy link
Contributor

This is rework of test for PR #1990

To check if a field is dirty we can simply use fromPHP() function of the mongodb driver. Besides, we shouldn't use the SQL-specific logic of Illuminate\Eloquent\Model::originalIsEquivalent() for mongodb. In sql, fields keep their schema data types and changing integer 1 to string '1' actually does nothing. However, in mongodb fields accept their types as they are inserted/updated. Therefore, if we change value of a field from (int)1 to (string)'1' we actually must regard it as dirty. This change is backward incompatible. Don't know if anyone likes the current behaviour more.

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07% 🎉

Comparison is base (e652b0c) 90.64% compared to head (2a2bb47) 90.72%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@ Coverage Diff @@## master #2515 +/- ## ============================================ + Coverage 90.64% 90.72% +0.07% + Complexity 758 754 -4  ============================================ Files 34 34 Lines 1849 1843 -6 ============================================ - Hits 1676 1672 -4 + Misses 173 171 -2 
Files ChangedCoverage Δ
src/Eloquent/Model.php90.90% <100.00%> (+0.75%)⬆️

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

Copy link
Member

@GromNaNGromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will leverage the MongoDB driver to detect attribute changes. It will reduce bugs and make the maintenance easier.
Could you rebase onto master and add a changelog entry?

Co-Authored-By: Jérôme Tamarelle <GromNaN@users.noreply.github.com> Co-Authored-By: Hamid Alaei Varnosfaderani <hamid.a85@gmail.com>
@divine
Copy link
ContributorAuthor

This change will leverage the MongoDB driver to detect attribute changes. It will reduce bugs and make the maintenance easier. Could you rebase onto master and add a changelog entry?

Hello,

Done! Can you please review it?

Thanks!

GromNaN
GromNaN previously approved these changes Aug 26, 2023
Copy link
Member

@GromNaNGromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

}

return is_numeric($attribute) && is_numeric($original)
&& strcmp((string) $attribute, (string) $original) === 0;
return fromPHP([$attribute]) === fromPHP([$original]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of notes on this:

  • fromPHP will be deprecated and is replaced by Document::fromPHP
  • Root-Level encoding is only supported for documents (i.e. any object or array with string keys)
  • If $attribute happens to be an object (e.g. an embedded document), this conversion will not yield the correct results as only public properties are encoded, which is most likely wrong for any Model
  • This may not be an issue, but I'll note that using this comparison ['foo' => 'bar', 'bar' => 'foo'] and ['bar' => 'foo', 'foo' => 'bar'] will not be treated as equal. That said, we don't have a good way to check BSON equality, and one may argue that two BSON documents with the same key/value pairs in different order shouldn't be considered equal.

For the time being, using this comparison may be feasible as long as we can ensure that it doesn't have side effects for embedded documents.

$this->assertEmpty($user->getDirty());

$user->birthday = new DateTime('19 august 1989');
$this->assertEmpty($user->getDirty());
}

public function testGetDirty()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test using embedded documents and arrays to ensure correct behaviour.

@GromNaNGromNaN added this to the 4.0.0 milestone Aug 30, 2023
@GromNaNGromNaN modified the milestones: 4.0, 4.1Dec 11, 2023
@GromNaN
Copy link
Member

Now that some transformations have been moved to the query builder (DateTimeInterface <=> UTCDateTime in #3119 and #3105), we might adjust to convert all dates before comparison.

@alcaeusalcaeus changed the base branch from 4.1 to 5.xSeptember 12, 2024 08:53
@alcaeusalcaeus requested a review from a team as a code ownerSeptember 12, 2024 08:53
@alcaeusalcaeus requested review from GromNaN and removed request for a teamSeptember 12, 2024 08:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants
@divine@codecov-commenter@GromNaN@jmikola@alcaeus
close