[Slim] Abstract API controllers

[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, 3.4.x, 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

Inspired by #426
Generator produces abstract API controller classes which needs to be extended by implementation classes in ./src folder.

When implementation class doesn't exist application throws exception:

How about implementing getPetById as a GET method in OpenAPIServer\Api\PetApi class?

Upgrade note

Do not modify abstract API controllers directly! Instead extend them by implementation classes like:

// src/Api/PetApi.php

namespace OpenAPIServer\Api;

use OpenAPIServer\Api\AbstractPetApi;

class PetApi extends AbstractPetApi
{

    public function addPet($request, $response, $args)
    {
        // your implementation of addPet method here
    }
}

Place all your implementation classes in ./src folder accordingly.
For instance, when abstract class located at ./lib/Api/AbstractPetApi.php you need to create implementation class at ./src/Api/PetApi.php.

Run $ composer dump-autoload every time your create new class.

To show error details with Slim modify index.php:

---$router = new SlimRouter();
+++$router = new SlimRouter(['settings' => ['displayErrorDetails' => true]]);

cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh

[ybelenko]

Few things that bothers me:

1. Maybe I shouldn't add CLI option to disable that feature? I think that maintain both templates will come with technical debt.
2. https://github.com/OpenAPITools/openapi-generator/blob/62de70be031eed6eb0891ba3de741235ecd5183e/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/PhpSlimServerCodegen.java#L180-L186 I've added Abstract prefix to {{classFilename}} and not changed {{classname}}. This approach helps me to display name of required implementation class within exceptions. Mayte it's not the best way, because when {{classFilename}} and {{classname}} are not the same it can lead to misunderstaing. The other way, I can add Abstract prefix to both {{classFilename}} and {{classname}} and add helper function that dynamically strips out Abstract prefix and returns required classname. But in this case I cannot print name of required implementation class in README or in any other mustaches.
3. What unit tests should we generate in current situation? Unit tests targeted to abstract classes seems a bit useless, because nobody will write tests for abstract classes. From another point, unit tests targeted to abstract classes can check that just generated build is not broken(at least Composer autoload works correct, namespaces are correct and method names). Generate unit tests for both implementation and abstract classes looks like overkill, am I wrong?

[ackintosh]

(I'm sorry if the comment below missing the point, I'm not familiar with Slim.)

1.

I prefer that doesn't add CLI option to disable that feature as I think it is better to keep options less and also we can do it later.

2.

Mayte it's not the best way, because when {{classFilename}} and {{classname}} are not the same it can lead to misunderstaing.

I prefer to avoid the misunderstanding via add "Abstract" prefix to both {{classFilename}} and {{classname}}.

and add helper function that dynamically strips out Abstract prefix and returns required classname. But in this case I cannot print name of required implementation class in README or in any other mustaches.

I think the helper function that dynamically strips out "Abstract" prefix isn't necessarily have to add. The concrete class name which implements the abstract class is determined by the users.

How about the simple exception message without {{classname}}? When the exception message is displayed, it means the abstract class is already extended by something concrete class. So stripping "Abstract" from {{classname}} may not very important.

"How about implementing call123TestSpecialTags as a PATCH method?"

3.

Totally agree with you.

[ybelenko]

@ackintosh

2. First, I want Composer to autoload src folder with user implementation files by default. https://github.com/OpenAPITools/openapi-generator/blob/62de70be031eed6eb0891ba3de741235ecd5183e/samples/server/petstore-security-test/php-slim/composer.json#L14-L17 It will autoload all user classes, but we don't know which class maps to each api controller. To solve this issue, I assume that user will implement {{classname}} class and will save it into src folder. My second goal is to avoid SlimRouter class editing. Please, take a look at https://github.com/OpenAPITools/openapi-generator/blob/62de70be031eed6eb0891ba3de741235ecd5183e/samples/server/petstore-security-test/php-slim/lib/SlimRouter.php#L94-L96 That script checks autoloaded {{classname}} class under app namespace and use it instead of abstract class. I think that force user to stick with predefined {{classname}} is very important, because it's only way to generate unit tests targeted to user classes.

3. Can't quite understand you. Please, describe your answer a bit more.

[ackintosh]

2.

Thanks for the details! I understand the importance that force user to stick with predefined {{classname}}.
Hmm...

Quick idea: How about indicate "Abstract" in namespace (folder name) instead of class name?

// lib/Api/AbstractFakeApi.php
namespace OpenAPIServer\Api;

abstract class AbstractFakeApi
{
}

↓↓↓

// lib/Abstracts/FakeApi.php
namespace OpenAPIServer\Abstracts;

abstract class FakeApi
{
}

3.

I'm sorry I didn't explain enough. 💦

I agree with that generate unit tests for both implementation and abstract classes is overkill.

Generating unit tests only for implementation class (predefined {{classname}}) looks good to me.

[ybelenko]

Quick idea: How about indicate "Abstract" in namespace (folder name) instead of class name?

I was inspited by PSR Naming Conventions. I think it's a good practice.
Quote:

Abstract classes MUST be prefixed by Abstract: e.g. Psr\Foo\AbstractBar.

[ackintosh]

Yeah, I don't think also put "Abstract" in namespace is best way. It's just an idea while thinking about {{classname}} issue.

---

Another idea: How about force the prefix as Abstract instead of using {{abstractNamePrefix}}?

The naming conventions says "Abstract classes MUST be prefixed by Abstract".

[ybelenko]

Another idea: How about force the prefix as Abstract instead of using {{abstractNamePrefix}}?

Actually, right now we don't have an option to change {{abstractNamePrefix}}. It's Abstract in all cases.:smile:

By the way, additional property {{implementationClassname}} to each api object can solve this issue without php helper function. The only thing is that @wing328 doesn't recommend to add additional properties without significant reason.

[ybelenko]

There are two ways to solve this issue:

1. {{classname}} = 'PetApi' and {{classFilename}} = 'AbstractPetApi'. And that can get user confused, because all of them expect the same {{classname}} and {{classFilename}}. User assumes that these props are interchangeable in PHP.
2. {{classname}} = 'AbstractPetApi' and {{classFilename}} = 'AbstractPetApi' with additional {{#vendorExtensions}}{{implementationClassname}} = 'PetApi'. Seems perfect to me, but yeah, it adds one more property to codegen.

I've ended up that PHP helper function to parse implementation classname from abstract classname isn't enough, because there will be many cases when that implementation classname required in mustaches(readme.md etc.), so it should be in codegen itself.

UPD: {{implementationClassname}} is too long, maybe {{userClassname}} is more handy.

[ybelenko]

Feature branch has been rebased on latest 4.0.x branch.
I've added {{userClassname}} property to operations codegen.
Props {{classname}} and {{classFilename}} are equal now.
All tests(phpunit, phplint, codesniffer) passed successfully.
It seems to me that all works like a charm.

Please, review that PR, because I need to submit two more before 4.0.0 release.

[ybelenko]

This PR has been submitted before @renepardon joined community, but I think he can review this changes too.
Of course, if he wants to and not very busy at the moment.

[ackintosh]

Looks good to me 👍

[ackintosh]

Oh, this PR may have to target master as now master branch is 4.0.0.

[ackintosh]

👍 👍