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

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

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

Conversation

divine
Copy link
Contributor

@divine divine commented Feb 19, 2023

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 Changed Coverage Δ
src/Eloquent/Model.php 90.90% <100.00%> (+0.75%) ⬆️

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

Copy link
Member

@GromNaN GromNaN 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?

src/Eloquent/Model.php Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
src/Eloquent/Model.php Outdated Show resolved Hide resolved
Co-Authored-By: Jérôme Tamarelle <[email protected]>
Co-Authored-By: Hamid Alaei Varnosfaderani <[email protected]>
@divine
Copy link
Contributor Author

divine commented Aug 26, 2023

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

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

LGTM.

CHANGELOG.md Outdated Show resolved Hide resolved
}

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.

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

GromNaN commented Sep 4, 2024

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.

@alcaeus alcaeus changed the base branch from 4.1 to 5.x September 12, 2024 08:53
@alcaeus alcaeus requested a review from a team as a code owner September 12, 2024 08:53
@alcaeus alcaeus requested review from GromNaN and removed request for a team September 12, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants