-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
Allow to use custom AR constructors #14078
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,10 +36,11 @@ public function getDoes() | |
} | ||
|
||
/** | ||
* @param type $row | ||
* | ||
* @param array $row | ||
* @return \yiiunit\data\ar\Animal | ||
*/ | ||
public static function instantiate($row) | ||
public static function instantiate($row = []) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bc break since it changes the method signature for everyone already extending this method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I noted about it in PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
$class = $row['type']; | ||
return new $class(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
<?php | ||
namespace yiiunit\data\ar; | ||
|
||
use yiiunit\framework\db\ActiveRecordTest; | ||
|
||
/** | ||
* Class Customer | ||
* | ||
* @property int $id | ||
* @property string $name | ||
* @property string $email | ||
* @property string $address | ||
* @property int $status | ||
* | ||
* @property ProfileWithConstructor $profile | ||
*/ | ||
class CustomerWithConstructor extends ActiveRecord | ||
{ | ||
const STATUS_ACTIVE = 1; | ||
const STATUS_INACTIVE = 2; | ||
|
||
public static function tableName() | ||
{ | ||
return 'customer'; | ||
} | ||
|
||
public function __construct($name, $email, $address) | ||
{ | ||
$this->name = $name; | ||
$this->email = $email; | ||
$this->address = $address; | ||
parent::__construct(); | ||
} | ||
|
||
public static function instantiate($row = []) | ||
{ | ||
return (new \ReflectionClass(static::className()))->newInstanceWithoutConstructor(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is because normal constructor has business logic, restrictions and events: public function __construct($name, $email)
{
if (empty($name) || empty($email)) {
throw new \InvalidArgumentException(...);
}
$this->name = $name;
$this->email = $email;
$this->created_at = time();
$this->recordEvent(new CustomerCreated($this));
parent::__construct();
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. edit: nvm this is not going into the core so lets not discuss it. i think thats up for the developer to know how to handle the constructor and the instantiate. lets think it the other way around. what if the developer assumes some property, behavior or something got initialized by the constructor and start using it after a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If developer really wants to use own customized code and knows how to override
By default |
||
} | ||
|
||
public function getProfile() | ||
{ | ||
return $this->hasOne(ProfileWithConstructor::className(), ['id' => 'profile_id']); | ||
} | ||
|
||
public function afterSave($insert, $changedAttributes) | ||
{ | ||
ActiveRecordTest::$afterSaveInsert = $insert; | ||
ActiveRecordTest::$afterSaveNewRecord = $this->isNewRecord; | ||
parent::afterSave($insert, $changedAttributes); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
<?php | ||
|
||
namespace yiiunit\data\ar; | ||
|
||
/** | ||
* Class OrderItem | ||
* | ||
* @property int $order_id | ||
* @property int $item_id | ||
* @property int $quantity | ||
* @property string $subtotal | ||
*/ | ||
class OrderItemWithConstructor extends ActiveRecord | ||
{ | ||
public static $tableName; | ||
|
||
public static function tableName() | ||
{ | ||
return static::$tableName ?: 'order_item'; | ||
} | ||
|
||
public function __construct($item_id, $quantity) | ||
{ | ||
$this->item_id = $item_id; | ||
$this->quantity = $quantity; | ||
parent::__construct(); | ||
} | ||
|
||
public static function instantiate($row = []) | ||
{ | ||
return (new \ReflectionClass(static::className()))->newInstanceWithoutConstructor(); | ||
} | ||
|
||
public function getOrder() | ||
{ | ||
return $this->hasOne(OrderWithConstructor::className(), ['id' => 'order_id']); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
<?php | ||
|
||
namespace yiiunit\data\ar; | ||
|
||
/** | ||
* Class Order | ||
* | ||
* @property int $id | ||
* @property int $customer_id | ||
* @property int $created_at | ||
* @property string $total | ||
* | ||
* @property OrderItemWithConstructor $orderItems | ||
* @property CustomerWithConstructor $customer | ||
* @property CustomerWithConstructor $customerJoinedWithProfile | ||
*/ | ||
class OrderWithConstructor extends ActiveRecord | ||
{ | ||
public static $tableName; | ||
|
||
public static function tableName() | ||
{ | ||
return static::$tableName ?: 'order'; | ||
} | ||
|
||
public function __construct($id) | ||
{ | ||
$this->id = $id; | ||
$this->created_at = time(); | ||
parent::__construct(); | ||
} | ||
|
||
public static function instantiate($row = []) | ||
{ | ||
return (new \ReflectionClass(static::className()))->newInstanceWithoutConstructor(); | ||
} | ||
|
||
public function getCustomer() | ||
{ | ||
return $this->hasOne(CustomerWithConstructor::className(), ['id' => 'customer_id']); | ||
} | ||
|
||
public function getCustomerJoinedWithProfile() | ||
{ | ||
return $this->hasOne(CustomerWithConstructor::className(), ['id' => 'customer_id']) | ||
->joinWith('profile'); | ||
} | ||
|
||
public function getOrderItems() | ||
{ | ||
return $this->hasMany(OrderItemWithConstructor::className(), ['order_id' => 'id'])->inverseOf('order'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
namespace yiiunit\data\ar; | ||
|
||
/** | ||
* Class Profile | ||
* | ||
* @property int $id | ||
* @property string $description | ||
* | ||
*/ | ||
class ProfileWithConstructor extends ActiveRecord | ||
{ | ||
public static function tableName() | ||
{ | ||
return 'profile'; | ||
} | ||
|
||
public function __construct($description) | ||
{ | ||
$this->description = $description; | ||
parent::__construct(); | ||
} | ||
|
||
public static function instantiate($row = []) | ||
{ | ||
return (new \ReflectionClass(static::className()))->newInstanceWithoutConstructor(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to have this one as is and, additionally, add another method w/o params that is creating an instance but isn't filling it. It would be used for cases when we're getting metadata such as attr. labels or joinWith relation metadata.
In 2.1 we may call the one w/o arguments
instantiate()
and the one w/$row
hydrate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need suggestions for method names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yiisoft/core-developers what do you think about naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @samdark ,
I think having
instantiate
method as is and add a new method that should simply create an empty model for metadata is the best approach right now.Right now people use "instantiate" methods to instantiate models and don't expect changes in the method signature or it's meaning.
But these two methods have different meanings and, from my point of view, should not share "instantiate" word in names.
So I suggest having a method that creates empty models with a name such as:
People probably would want to use this method passing instantiation stuff so I don't think "internal" or something similar should be used in this method.
Also, if skip options with "Model", I think, it makes sense to move method to some of the base classes.
For example,
createNew
might be implemented in Object.Object::createNew
is pretty natural from my point of view.So what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create()
actually sounds good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ElisDN what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer
Model::new()
- it is closer to native PHP syntax and IMO better fits for current naming convention.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unlikely posssible since
new
is PHP reserved keyword.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works in PHP7, so we could use it if we target this to Yii 2.1.
https://3v4l.org/v8vAv