[PHP] Decommission "packagePath", add new option "packageName" #681

wing328 · 2018-07-29T08:34:45Z

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

Decommission "packagePath"
Users will need to use -o (--output) to control the destination folder instead to make it consistent with other generators
add new option "packageName"

To close #663

cc @jebentier (2017/07) @dkarlovi (2017/07) @mandrean (2017/08) @jfastnacht (2017/09) @ackintosh (2017/09) @ybelenko (2018/07)

ybelenko

I've used getPackagePath method in

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/PhpSlimServerCodegen.java

Lines 113 to 120 in 926b971

    
           supportingFiles.add(new SupportingFile("README.mustache", getPackagePath(), "README.md")); 
        
           supportingFiles.add(new SupportingFile("composer.mustache", getPackagePath(), "composer.json")); 
        
           supportingFiles.add(new SupportingFile("index.mustache", getPackagePath(), "index.php")); 
        
           supportingFiles.add(new SupportingFile(".htaccess", getPackagePath(), ".htaccess")); 
        
           supportingFiles.add(new SupportingFile(".gitignore", getPackagePath(), ".gitignore")); 
        
           supportingFiles.add(new SupportingFile("AbstractApiController.mustache", toSrcPath(invokerPackage, srcBasePath), "AbstractApiController.php")); 
        
           supportingFiles.add(new SupportingFile("SlimRouter.mustache", toSrcPath(invokerPackage, srcBasePath), "SlimRouter.php")); 
        
           supportingFiles.add(new SupportingFile("phpunit.xml.mustache", getPackagePath(), "phpunit.xml.dist"));

You should change it too to keep all codegens consistent.

wing328 · 2018-07-31T02:53:57Z

@ybelenko what about keeping getPackagePath() or packagePath in all PHP server generators and put the following in the constructor instead?

        packagePath = ""; // empty packagePath (top folder)

so that the package path (root folder) can be changed easily by the users if they need to customize it somehow?

ybelenko · 2018-07-31T11:07:42Z

@wing328 It was my first approuch. I don't have full knowledge of your path generation, but this way I can generate code in a specific folder. I think we need to ask @ackintosh about that.

wing328 · 2018-07-31T13:52:38Z

@ybelenko sure let's wait for @ackintosh to review.

Btw, the following approach is already used in other PHP server generators:

        packagePath = ""; // empty packagePath (top folder)

ackintosh · 2018-08-02T13:39:36Z

I think packagePath = "" is good idea. 💡

It sets the destination to root folder as default and be able to change if user want.

wing328 · 2018-08-02T13:41:29Z

@ackintosh thanks for the feedback. I'll change the rest accordingly.

wing328 · 2018-08-04T16:47:22Z

@ybelenko @ackintosh I've decommissioned packagePath and add a new option "packageName". The samples are exactly the same as before. Please review when you've time.

ackintosh

Found a -o target which should be fixed. 👀

https://github.com/ackintosh/openapi-generator-1/blob/27426f7b522c1b55f6ebe6c90d41d77e3c7ab589/bin/openapi3/php-silex-petstore-server.sh#L30

samples/server/petstore/php-silex -> samples/server/petstore/php-silex/OpenAPIServer

wing328 · 2018-08-05T05:07:28Z

@ackintosh good catch. Fixed via 0d45184

ackintosh

Looks good to me. 👍

wing328 · 2018-08-05T06:49:04Z

@ybelenko can you merge it if it looks good to you? Thanks

ybelenko · 2018-08-05T23:31:16Z

Now I don't understand what we need packageName option for? We don't use it in path construction.

The only reason I can imagine for packageName("GeneratedPetstore" packageName as example):

/**
 * FakeApi Class Doc Comment
 * PHP Version 5
 *
 * @category Class
 * @package  GeneratedPetstore
 * @author   OpenAPI Generator team
 */
class FakeApi {

Do we keep packageName option to avoid breaking changes as fallback solution?
Or do you expect that user wants to use packageName in mustaches somehow?

The main package name for classes. e.g. GeneratedPetstore

If I understand this description correct, then mustache should look like:

class {{packageName}}{{classname}}

and result:

class GeneratedPetstoreFakeApi

In total packageName is a prefix to generated class names, isn't it?

wing328 · 2018-08-06T08:36:06Z

The {{packageName}} is used in the README: https://github.com/OpenAPITools/openapi-generator/pull/681/files#diff-5763a353c6e95ff8967e09ce0e52ba53R1

wing328 · 2018-08-06T10:14:21Z

Upgrade Note

The packagePath option has been deprecated. Please use -o or --output to specify the output folder.

To specify the package name in the auto-generated README.md, please use the new option packageName instead.

Seems like issue OpenAPITools#663 and pull request OpenAPITools#681 missed this security script, I've changed output path in bin/security/silex-petstore-server.sh.

@ackintosh

* [PHP] Remove PHP related rules from root gitignore After conversation with @ackintosh we've agreed that every PHP codegen should create it's own `.gitignore`. Client libraries(SDKs) should ignore `composer.lock` file while server stubs better do opposite. * [PHP] Set .gitignore as default supporting file * [PHP] Add default gitignore to Client SDK * [PHP] Add default gitignore to Laravel * [PHP] Add default gitignore to Lumen * [PHP] Add default gitignore to Silex Seems like issue #663 and pull request #681 missed this security script, I've changed output path in bin/security/silex-petstore-server.sh. * [PHP] Update Slim * [PHP] Add default gitignore to Symfony I've copied few old rules from root gitignore to local one. These rules should be reviewed by original SymfonyCodegen authors. * [PHP] Add default gitignore to Zend * [PHP] Refresh Openapi3 client samples * [PHP] Add refs to .gitignore templates collection

…PITools#681) * remove packagePath from php file location, use -o instead * fix php symfony top folder * restore pacakgePath * update php laraavel samples * remove packagePath from PHP generator * add new silex files * update window batch - php silex * fix openapi3 silex script

@ackintosh

* [PHP] Remove PHP related rules from root gitignore After conversation with @ackintosh we've agreed that every PHP codegen should create it's own `.gitignore`. Client libraries(SDKs) should ignore `composer.lock` file while server stubs better do opposite. * [PHP] Set .gitignore as default supporting file * [PHP] Add default gitignore to Client SDK * [PHP] Add default gitignore to Laravel * [PHP] Add default gitignore to Lumen * [PHP] Add default gitignore to Silex Seems like issue OpenAPITools#663 and pull request OpenAPITools#681 missed this security script, I've changed output path in bin/security/silex-petstore-server.sh. * [PHP] Update Slim * [PHP] Add default gitignore to Symfony I've copied few old rules from root gitignore to local one. These rules should be reviewed by original SymfonyCodegen authors. * [PHP] Add default gitignore to Zend * [PHP] Refresh Openapi3 client samples * [PHP] Add refs to .gitignore templates collection

remove packagePath from php file location, use -o instead

da133ab

wing328 added Client: PHP Enhancement: General Server: PHP labels Jul 29, 2018

wing328 added this to the 3.2.0 milestone Jul 29, 2018

fix php symfony top folder

7a122bc

wing328 mentioned this pull request Jul 29, 2018

[PHP] Generate SDK into root directory not into a folder #663

Closed

wing328 added the Breaking change (with fallback) label Jul 29, 2018

ybelenko requested changes Jul 30, 2018

View reviewed changes

wing328 added 6 commits August 3, 2018 14:20

restore pacakgePath

a4fb7e2

Merge remote-tracking branch 'origin/master' into php_output

ad116ec

update php laraavel samples

94ca5d9

remove packagePath from PHP generator

ae97ed9

add new silex files

2600d06

update window batch - php silex

42a1332

wing328 changed the title ~~Do not use "packagePath" when setting PHP file path~~ [PHP] Decommission "packagePath", add new option "packageName Aug 4, 2018

wing328 changed the title ~~[PHP] Decommission "packagePath", add new option "packageName~~ [PHP] Decommission "packagePath", add new option "packageName" Aug 4, 2018

ackintosh reviewed Aug 5, 2018

View reviewed changes

fix openapi3 silex script

0d45184

ackintosh approved these changes Aug 5, 2018

View reviewed changes

ybelenko approved these changes Aug 5, 2018

View reviewed changes

wing328 merged commit c116c8f into master Aug 6, 2018

wing328 deleted the php_output branch August 6, 2018 10:12

ackintosh mentioned this pull request Aug 9, 2018

[PHP] Fix deserialize ApiException as a Model #757

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PHP] Decommission "packagePath", add new option "packageName" #681

[PHP] Decommission "packagePath", add new option "packageName" #681

wing328 commented Jul 29, 2018 •

edited

Loading

ybelenko left a comment

wing328 commented Jul 31, 2018

ybelenko commented Jul 31, 2018

wing328 commented Jul 31, 2018

ackintosh commented Aug 2, 2018

wing328 commented Aug 2, 2018

wing328 commented Aug 4, 2018

ackintosh left a comment •

edited

Loading

wing328 commented Aug 5, 2018

ackintosh left a comment

wing328 commented Aug 5, 2018

ybelenko commented Aug 5, 2018

wing328 commented Aug 6, 2018

wing328 commented Aug 6, 2018

	supportingFiles.add(new SupportingFile("README.mustache", getPackagePath(), "README.md"));
	supportingFiles.add(new SupportingFile("composer.mustache", getPackagePath(), "composer.json"));
	supportingFiles.add(new SupportingFile("index.mustache", getPackagePath(), "index.php"));
	supportingFiles.add(new SupportingFile(".htaccess", getPackagePath(), ".htaccess"));
	supportingFiles.add(new SupportingFile(".gitignore", getPackagePath(), ".gitignore"));
	supportingFiles.add(new SupportingFile("AbstractApiController.mustache", toSrcPath(invokerPackage, srcBasePath), "AbstractApiController.php"));
	supportingFiles.add(new SupportingFile("SlimRouter.mustache", toSrcPath(invokerPackage, srcBasePath), "SlimRouter.php"));
	supportingFiles.add(new SupportingFile("phpunit.xml.mustache", getPackagePath(), "phpunit.xml.dist"));

[PHP] Decommission "packagePath", add new option "packageName" #681

[PHP] Decommission "packagePath", add new option "packageName" #681

Conversation

wing328 commented Jul 29, 2018 • edited Loading

PR checklist

Description of the PR

ybelenko left a comment

Choose a reason for hiding this comment

wing328 commented Jul 31, 2018

ybelenko commented Jul 31, 2018

wing328 commented Jul 31, 2018

ackintosh commented Aug 2, 2018

wing328 commented Aug 2, 2018

wing328 commented Aug 4, 2018

ackintosh left a comment • edited Loading

Choose a reason for hiding this comment

wing328 commented Aug 5, 2018

ackintosh left a comment

Choose a reason for hiding this comment

wing328 commented Aug 5, 2018

ybelenko commented Aug 5, 2018

wing328 commented Aug 6, 2018

wing328 commented Aug 6, 2018

Upgrade Note

wing328 commented Jul 29, 2018 •

edited

Loading

ackintosh left a comment •

edited

Loading