Skip to content
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

[PHP][php-ze-ph] Support for PHP 7.1+, Zend Expressive 3.2 and PathHander 0.4 #1902

Merged
merged 5 commits into from
Jan 30, 2019
Merged

[PHP][php-ze-ph] Support for PHP 7.1+, Zend Expressive 3.2 and PathHander 0.4 #1902

merged 5 commits into from
Jan 30, 2019

Conversation

Articus
Copy link
Contributor

@Articus Articus commented Jan 13, 2019

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.

@jebentier, @dkarlovi , @mandrean , @jfastnacht , @ackintosh , @ybelenko , @renepardon

Description of the PR

Update for php-ze-ph server generator - switch to PHP 7.1 for #1246 and corresponding dependency update.
And I also believe that current generated README.md for php-ze-ph complies with suggestions here.

@auto-labeler
Copy link

auto-labeler bot commented Jan 13, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@ybelenko
Copy link
Contributor

CircleCI says that there are uncomitted files in:
samples/server/petstore/php-ze-ph/src/App/Factory.php
samples/server/petstore/php-ze-ph/src/App/Middleware/
Please, regenerate your server and commit these files to fix CI job.

@ybelenko
Copy link
Contributor

It seems to me as breaking change without fallback (PHP 7.1 and newer required) correct me if I'm wrong. Could you write any upgrade note for users? If it's already exists in generated README just copy it to current discussion.

@Articus
Copy link
Contributor Author

Articus commented Jan 14, 2019

CircleCI says that there are uncomitted files in:
samples/server/petstore/php-ze-ph/src/App/Factory.php
samples/server/petstore/php-ze-ph/src/App/Middleware/
Please, regenerate your server and commit these files to fix CI job.

Oops, sorry - forgot to delete samples folder before regenerating, so obsolete generated files were kept and new files were not added :[ Should be fixed now.

It seems to me as breaking change without fallback (PHP 7.1 and newer required)

Yes, that is correct. Updated generator creates server stub based on Zend Expressive 3.2 and PathHandler 0.4 . They both require PHP 7.1+ and are backward incompatible with their previous versions Zend Expressive 2.* and PathHandler 0.3 that were supported by php-ze-ph before. Hopefully it is not a huge issue though - this generator was meant to generate skeleton for new project, not ready-to-use library (like php-client for example).
So if you want ot migrate existing project, it is better to check migration notes for both libraries - here and here.

@ybelenko
Copy link
Contributor

Sorry, for long delay. Tried to test locally. I had to install ext-yaml, but it's not mentioned in README.md. Then my installation failed with:

$ composer install
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.                                                                              
  Problem 1
    - zendframework/zend-httphandlerrunner 1.0.1 requires psr/http-message-implementation ^1.0 -> no matching package found.
    - articus/path-handler 0.4 requires zendframework/zend-expressive ^3.2 -> satisfiable by zendframework/zend-expressive[3.2.0, 3.2.1].
    - zendframework/zend-expressive 3.2.0 requires zendframework/zend-httphandlerrunner ^1.0.1 -> satisfiable by zendframework/zend-httphandlerrunner[1.0.1].
    - zendframework/zend-expressive 3.2.1 requires zendframework/zend-httphandlerrunner ^1.0.1 -> satisfiable by zendframework/zend-httphandlerrunner[1.0.1].
    - Installation request for articus/path-handler ^0.4 -> satisfiable by articus/path-handler[0.4].

My config:
OS: Windows 8.1
PHP 7.2.7 (cli) (built: Jun 19 2018 23:14:45)

Please, update composer.json to install missed dependencies.

Arthur Mogliev added 2 commits January 26, 2019 15:28
- overwriting "*/*" media type for producers with "n/a" (PathHandler does not support that cause it makes no sense to return response with "Content-Type: */*")
- "array" return type declaration for handler methods with ambiguous "container" return type
- better way to generate attribute annotation stub for request body data with ambiguous "container" type
- fixed missing dependency in composer.json
- minor optimization for container.php
- samples for OAS3 petstore spec
- note about ext-yaml in stub README
- updated .gitignore
@Articus
Copy link
Contributor Author

Articus commented Jan 26, 2019

Sorry, for long delay.

No problem, thanks for thorough review! Rushing with PR on Sunday evening was a terrible idea - made really silly and embarrassing mistakes :[
Delay allowed to cool off a bit and retest generated stub properly - added few more minor improvements. Hopefully should be ok to merge now :)

@ybelenko
Copy link
Contributor

I've checked build locally and everything looks fine. All tested endpoints responded with 501 Not Implemented which is correct accordingly to README.md. I didn't check code with PHPCodeSniffer because this should be accomplished in next PRs.

Small notice, I'm embarrased, but I didn't know that 0.0.0.0 host need to be called as localhost. Maybe README.md should point that or suggest to use php -S localhost:8080 -t ./public instead.

@Articus
Copy link
Contributor Author

Articus commented Jan 27, 2019

Small notice, I'm embarrased, but I didn't know that 0.0.0.0 host need to be called as localhost. Maybe README.md should point that or suggest to use php -S localhost:8080 -t ./public instead.

I believe current variant php -S 0.0.0.0:8080 -t ./public is more versatile because it will start dev server on all network interfaces while php -S localhost:8080 -t ./public will start dev server only on loopback network interface that usually has IP 127.0.0.1. So if PHP CLI is installed on your machine both variants will work. But if PHP CLI is inside Docker container for example (like I usually do :) ), you will be able to reach only dev server started with php -S 0.0.0.0:8080 -t ./public.

- logging '*/*' replacement as warning
@ybelenko
Copy link
Contributor

Maybe README.md should point

@Articus I'm not asking you to change host, just describe it in the readme how user can reach API. I thought that build is broken when I cannot reach http://0.0.0.0:8080 in Google Chrome. This misunderstaning can cause a lot of issues.

@Articus
Copy link
Contributor Author

Articus commented Jan 28, 2019

Sorry, but I do believe that current information in README:

  • start PHP development server: php -S 0.0.0.0:8080 -t ./public (or any other SAPI you prefer, just make sure that you configure webroot to ./public and rewrites to ./public/index.php)

is enough for any PHP developer capable of making complete application from stub because the way you access API at this point entirely depends on your development environment and your programming habits.

@ybelenko
Copy link
Contributor

is enough for any PHP developer capable of making complete application

@Articus I've spend about 30-minutes to figured out why 0.0.0.0:8080 doesn't work in browser. Anyway, forget about it, maybe I'm just a low skill developer.

@wing328 wing328 merged commit 77d2de4 into OpenAPITools:master Jan 30, 2019
@wing328 wing328 added this to the 4.0.0 milestone Jan 30, 2019
@wing328
Copy link
Member

wing328 commented Jan 30, 2019

Tweet to promote the change: https://twitter.com/oas_generator/status/1090516974549909504

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…nder 0.4 (OpenAPITools#1902)

* - support for PHP 7.1, Zend Expressive 3.2 and PathHander 0.4 for php-ze-ph generator

* - fixed mess with petstore samples (added new files, removed obsolete files)

* php-ze-ph:
- overwriting "*/*" media type for producers with "n/a" (PathHandler does not support that cause it makes no sense to return response with "Content-Type: */*")
- "array" return type declaration for handler methods with ambiguous "container" return type
- better way to generate attribute annotation stub for request body data with ambiguous "container" type
- fixed missing dependency in composer.json
- minor optimization for container.php
- samples for OAS3 petstore spec

* php-ze-ph:
- note about ext-yaml in stub README
- updated .gitignore

* php-ze-ph:
- logging '*/*' replacement as warning
@ybelenko ybelenko mentioned this pull request Jun 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants