Skip to content
GitLab
Projects Groups Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
  • O openapi-generator
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Issues 3,476
    • Issues 3,476
    • List
    • Boards
    • Service Desk
    • Milestones
  • Merge requests 402
    • Merge requests 402
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages and registries
    • Packages and registries
    • Package Registry
    • Infrastructure Registry
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • OpenAPI Tools
  • openapi-generator
  • Merge requests
  • !735

[Slim3] Models constructor

  • Review changes

  • Download
  • Email patches
  • Plain diff
Closed Administrator requested to merge github/fork/ybelenko/slim_models into master Aug 03, 2018
  • Overview 0
  • Commits 5
  • Pipelines 0
  • Changes 38

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:

  1. It's more common and expected for developers, more descriptive, you can say what properties exists with a quick look
  2. 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.
  3. 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
  4. 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:

  1. 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.
  2. 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. 😕 The question is, should we split arguments to required and optional and throw exceptions right from constructor method OR set all arguments optional and validates them silently with special method valid() only when user ask for it?

Issues found in process

  1. Parser optionalVars.hasMore in models, #710 (closed) 🐛
  2. [Slim] Security samples script fails, #734 (closed)

cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh

Assignee
Assign to
Reviewers
Request review from
Time tracking
Source branch: github/fork/ybelenko/slim_models