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

PHPORM-180 Keep createOrFirst in 2 commands to simplify implementation #2984

Merged
merged 4 commits into from
May 30, 2024

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented May 29, 2024

Fix PHPORM-180
Alternative to #2980

Revert optimisation from #2742 in order to keep all the processes from Eloquent and simplify maintenance.

The main feature difference is that it's requiring a unique index whereas my previous implementation was only using the filter "attributes". This new implementation is closer to Laravel's implementation, it only removes the nested transaction and catch the MongoDB exception instead of the PDO exception.

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

@GromNaN GromNaN requested a review from a team as a code owner May 29, 2024 10:29
@GromNaN GromNaN requested review from alcaeus and jmikola and removed request for alcaeus May 29, 2024 10:29
@GromNaN GromNaN added this to the 4.4 milestone May 29, 2024
@GromNaN
Copy link
Member Author

GromNaN commented May 29, 2024

Unfortunately, that doesn't solve the nested transaction issue reported initially #2720.

MongoDB\Driver\Exception\CommandException: Transaction with { txnNumber: 1 } has been aborted.

@GromNaN GromNaN force-pushed the PHPORM-180-revert branch 2 times, most recently from 65418d2 to 181f2de Compare May 29, 2024 14:15
src/Eloquent/Builder.php Outdated Show resolved Hide resolved
src/Eloquent/Builder.php Show resolved Hide resolved
tests/ModelTest.php Show resolved Hide resolved
tests/ModelTest.php Show resolved Hide resolved
tests/ModelTest.php Show resolved Hide resolved
* @param array $values The attributes to insert if no matching record is found
*/
public function createOrFirst(array $attributes = [], array $values = []): Model
public function firstOrCreate(array $attributes = [], array $values = [])
Copy link
Member

Choose a reason for hiding this comment

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

I'm not actually suggesting this, but it's amusing to me that this entire method body could be reduced to:

return (clone $this)->where($attributes)->first() ??
    $this->getConnection()->getSession()?->isInTransaction()
        ? $this->create(array_merge($attributes, $values))
        : $this->createOrFirst($attributes, $values);

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but it won't help me to use xdebug's step-by-step debugger.

@GromNaN GromNaN merged commit 55ca36e into mongodb:4.4 May 30, 2024
26 checks passed
@GromNaN GromNaN deleted the PHPORM-180-revert branch May 30, 2024 16:01
@GromNaN GromNaN added the bug label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants