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 7.3 v2 Specialization #1110

Merged

Conversation

AlbertoLopezBenito
Copy link
Contributor

@AlbertoLopezBenito AlbertoLopezBenito commented Mar 1, 2019

Noticed in #807

This PR providers support for the v2 specialization for PHP. I based it off the python package to do it.
Features added:

  • Fission v2 support
  • PHP 7.3 from 7.1
  • Removed ZendServer usage (deprecated)
  • Removed usage of built-in PHP server, is not recomended to use it in production.

NOTE: Fission v1 is still working, all changes are backward compatible


This change is Reviewable

@life1347 life1347 assigned life1347 and unassigned life1347 Mar 4, 2019
@life1347 life1347 self-requested a review March 4, 2019 08:07
@life1347
Copy link
Member

life1347 commented Mar 4, 2019

Hi @AlbertoLopezBenito Thanks for this PR! I've finished the review and think it's almost in good shaper, however, after some tests I have couple questions want to confirm:

  1. Is it possible for php-builder to do code validation during package build stage?

I tried with the following error source code (no <?$php at the beginning of file), and the function returns whole file content directly instead of an error. Is that a normal behavior for php? If not, could we enforce code validation during build stage (or in code loading stage)?

  1. No function error logs

If there is any syntax errors in source code, for example missing ;, the php server doesn't print any error logs. Is that normal or it's intentional?

  1. A simple example for php package usage (Optional)

It would be great if there is a simple example in fission/examples/php7 to demonstrate how to create a php package with composer.json. It helps people to understand how it actually works.

Thanks again for your work.

@AlbertoLopezBenito
Copy link
Contributor Author

Hi @life1347
I just uploaded a new commit checking syntax errors in source files and adding an example using the builder. It's the same example that python provides 😉

Related to

Is it possible for php-builder to do code validation during package build stage?

PHP renders the code and will execute the parts with <?php. It was designed for that purpose, so it's normal behavior.

Thanks for your review!

@life1347 life1347 added this to the 1.1 milestone Mar 6, 2019
@life1347
Copy link
Member

life1347 commented Mar 7, 2019

Looks like there is error in php server. Use example to test, and function works for the first time. Then, it crashed for the second time. Following is test logs.

poolmgr-php-default-ncnwzvk9-5657758556-pnsbr fetcher 2019/03/07 07:59:13 Successfully placed at /userfunc/e257b7da-40ae-11e9-b759-08002778d774
poolmgr-php-default-ncnwzvk9-5657758556-pnsbr fetcher 2019/03/07 07:59:13 Elapsed time in fetch request = 123.037967ms
poolmgr-php-default-ncnwzvk9-5657758556-pnsbr php [2019-03-07 07:59:13] Function.DEBUG: File read: example.txt [] []
poolmgr-php-default-ncnwzvk9-5657758556-pnsbr php Fatal error: Cannot redeclare execute() (previously declared in /userfunc/e257b7da-40ae-11e9-b759-08002778d774/handlers/FileReader.php:8) in /userfunc/e257b7da-40ae-11e9-b759-08002778d774/handlers/FileReader.php on line 16

return new Response(500, [], $codePath . ' - ' . $throwable->getMessage());
}

require $codePath;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's the root cause because server requires user function twice.

Copy link
Member

@life1347 life1347 Mar 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a flag to not require again if we already did that before. Or, any better suggestion?
How about do "require" when when specialize? So that we don't need extra flag.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After moving require to specialize stage it works perfectly. Once this problem is addressed then we can merge it! 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@life1347 Fixed! I changed require to require_once. This prevents that behavior :)

@life1347 life1347 merged commit f104f0b into fission:master Mar 7, 2019
@life1347
Copy link
Member

life1347 commented Mar 7, 2019

Thanks for all your works @AlbertoLopezBenito , merged! 🎉 💯 🥇

@vishal-biyani
Copy link
Member

Great job and thanks a lot @AlbertoLopezBenito 🎉

vishal-biyani pushed a commit that referenced this pull request Mar 13, 2019
* Added support for PHP 7.3 v2 Specialization
* Add builder dockerfile for PHP7
* Support mcrypt in PHP 7.3

use zap for logging

Signed-off-by: Jon Carl <grounded042@icloud.com>

update glide deps

Signed-off-by: Jon Carl <grounded042@icloud.com>

add missing logger

Signed-off-by: Jon Carl <grounded042@icloud.com>

fixes based on PR feedback

Signed-off-by: Jon Carl <grounded042@icloud.com>

fix all imports to be three paragraph

Signed-off-by: Jon Carl <grounded042@icloud.com>

fix log from pr merged into master

Signed-off-by: Jon Carl <grounded042@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants