Created by: ybelenko
PR checklist
-
Read the contribution guidelines. -
Ran the shell script under ./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
. -
Filed the PR against the correct branch: master
,4.0.x
. Default:master
. -
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.
Description of the PR
New model template generates file like:
class Animal
{
/** @var string $className */
private $className;
/** @var string $color (optional) Default 'red' */
private $color = 'red';
/**
* Animal constructor
*
* @param string $className
* @param string|null $color (optional) Default 'red'
*/
public function __construct(
$className,
$color = 'red'
) {
$this->className = $className;
$this->color = $color;
}
/**
* Alternative static class constructor
*
* @param mixed[]|null $data Associated array of property values initializing the model
* @throws InvalidArgumentException when $data doesn't contain required constructor arguments
* @example $animal = Animal::createFromObject([ 'className' => 'foobar' ]);
*
* @return Animal
*/
public static function createFromObject(array $data = null)
{
if ($data['className'] === null) {
throw new InvalidArgumentException("'className' can't be null");
}
$className = $data['className'];
$color = (isset($data['color'])) ? $data['color'] : 'red';
return new Animal(
$className,
$color
);
}
}
Why that way and not another
Why I think that private $className
and private $color
is better than single $container
from PHPClient codegen:
- It's more common and expected for developers, more descriptive, you can say what properties exists with a quick look
- Most of time developer needs to query database and map result to related model class. For this case there is PDOStatement::fetchObject(). Other database drivers(mysqli) has similar methods too.
- I've quickly checked popular ORMs. Eloquent have builtin generator to produce models and doesn't follow approuch from this PR. Doctrine however, creates similar models, example User.php. The last one Atlas claims own persistence models with optional fallback to domain models. That domain models looks similar to that PR, example Map From Persistence To Domain
- PhpSymfonyCodegen already produces that kind of models and it looks like a stable implementation, correct me if I'm wrong
Why I've added both traditional and associative array constructor in the same class:
- I cannot choose one, so I've decided to add
createFromObject
static method when you want to parse JSON and get new model instance as result. I like classic constructor with arguments list more, but I understand that sometimes it doesn't fit. - With classic constructor you can see required and optional variables right away.
Unresolved question to discuss
I can't say for sure that force user to set required arguments is a good idea. I mean sometimes you receive messy JSON object without required fields, if you'll try to parse it via createFromObject
method your code will die with uncatched exception. From another point, it's very usefull during development to notice missed properties instantly.
PHPClient throws exceptions when you're trying to set property with setter method, but it gives no exceptions within constructor even if you don't provide any props at all. It doesn't make sense to me. valid()
only when user ask for it?
Issues found in process
- Parser optionalVars.hasMore in models, #710 (closed)
🐛 - [Slim] Security samples script fails, #734 (closed)
cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh