PHPORM-180 Trigger events in Model::createOrFirst #2980
Conversation
GromNaN
force-pushed
the
PHPORM-180
branch
2 times, most recently
from
ca91f03
to
43e5023
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // If the model has an incrementing key, we can use the "insertGetId" method on | ||
| // the query builder, which will give us back the final inserted ID for this | ||
| // table from the database. Not all tables have to be incrementing though. | ||
| $attributes = $this->getAttributesForInsert(); |
There was a problem hiding this comment.
Why does the comment above refer to insertGetId()? It's not clear to me how that's getting used here, unless it's through getAttributesForInsert().
Does getAttributesForInsert respect modifications from earlier model methods such as setUniqueIds() and updateTimestamps()? I assume this all pulls from the same state.
|
|
||
| return $model; | ||
| }); | ||
| return $instance->saveOrFirst($attributes); |
There was a problem hiding this comment.
I don't recall the original reason for introducing createOrFirst() in #2742 (PHPORM-139). It looks like this was necessary to avoid some issue with transactions.
Just wanted to confirm that a custom implementation is still required at all, given that we'll no longer be using findAndModify.
There was a problem hiding this comment.
We are still using findAndModify, but in Query\Builder::findAndModify.
The call chain is:
Model::createOrFirstEloquent\Query::createOrFirstModel::saveOrFirstQuery\Builder::findAndModifyCollection::findAndModify
| /** @internal Not part of Laravel Eloquent API. Use raw findOneAndModify if necessary */ | ||
| public function saveOrFirst(array $criteria): ?static | ||
| { | ||
| $this->mergeAttributesFromCachedCasts(); |
There was a problem hiding this comment.
I assume much of the contents for this method were lifted from an existing method in Eloquent. If so, can we reference that in the method doc block?
I'm a little worried that if anything changes upstream we'll probably need to find out the hard way that this needs to be updated, but that may be unavoidable.
There was a problem hiding this comment.
I'm also concerned that we have a lot of duplicated code that is hard to understand and we are not sure we can maintain in the long-term.
Creating a simple alternative PR: #2984
|
|
||
| $connection = $query->getConnection(); | ||
| if (! $this->getConnectionName() && $connection) { | ||
| $this->setConnection($connection->getName()); |
There was a problem hiding this comment.
What's the reason for inheriting the query's connection? IS this necessary for dispatching the created event?
| } | ||
|
|
||
| // If the model is successfully saved, we need to do a few more things once | ||
| // that is done. We will call the "saved" method here to run any actions |
There was a problem hiding this comment.
I don't see any reference to a saved method. Is this comment accurate?
|
Replaced by #2984 |
Fix PHPORM-180
Fix #2935
In order to trigger the same events as Eloquent's
createOrFirst, the process must be in theModelclass. So I created a newsaveOrFirstmethod that needs to be public to be called by theEloquent\Queryclass, but I want to keep it internal. I tried to add an option toModel::save, but this method returns an boolean and I need the object if found.Checklist
Update documentation for new features