Remove Model::{asModel, saveAs, newInstance} methods#856
Conversation
src/Model.php
Outdated
| // Copying only non-default value | ||
| $m->set($field, $value); | ||
| } | ||
| $m->set($field, $value); |
There was a problem hiding this comment.
You can do simply $m->set($this->data); here without loop.
But I'm not sure why you removed condition lines above. There was some logic - to not copy null and default values and also of course not copy id.
There was a problem hiding this comment.
- ignoring null - why? (I think too much crazy logic)
- ignoring v = default - saves only a call, nothin else :) (should be removed)
- ignoring ID - why (seems wrong to me, different model usually still targets the same data/table)
We would then need loadAs... This is too much methods for nothing.
| $model = (self::class)::fromSeed([$class ?? static::class], $options); | ||
|
|
||
| if ($this->persistence) { | ||
| return $this->persistence->add($model); // @phpstan-ignore-line |
There was a problem hiding this comment.
This is the essence of newInstance(). At least for me.
It creates new model object and already links it with same persistence of old object.
There was a problem hiding this comment.
I am aware, I can not be fixed in the constructor as construtor is not aware of the previous instance/persistence.
The solution is to call new $class($model->persistence), eg. pass the persistence manually when needed.
Safety is handled, all DB operations throws if persistence is not set.
| inherit all the fields, methods and actions of "User" class but will introduce one new | ||
| action - `send_gift`. | ||
|
|
||
| There are some advanced techniques like "SubTypes" or class substitution, |
There was a problem hiding this comment.
And this was quite a nice feature we used in one project :)
There was a problem hiding this comment.
See code below, it tries to return different class from load method.
Notice, that a lot of custom code is needed already, you can adjust the code for the removed method easily. Usually, in the case below, you want to retain ID!
DarkSide666
left a comment
There was a problem hiding this comment.
Is George around in Discord sometime? I would love to hear his opinion too on this.
I kind of understand that we can live without these methods (including asModel), but on other hand - why should we? Everything can be done manually in project-specific way, but it's also good if framework does some work for you (at least in one specific way and if you don't like that, then you can overwrite or extend).
So to wrap this up - I will approve this PR, but please try to discuss it also with George before merging. Maybe he can come up with better example of use-case then I could.
|
@georgehristov Please approve as well, |
The removed methods were more or less confusing...
Model::asModelreplacement:see removed code, adjust for your usecase (that is why we removed this method, sometimes ID or default values should be copied, sometimes not)
Model::saveAs(string $class, array $options = [])replacement:Model::newInstance(string $class = null, array $options = [])replacement:This works even with
$classbeiing an object - https://3v4l.org/EfOiWThis PR also improve/simplify
Model::asModel()method.No change in
atk4/uirepo needed.