Skip to content
This repository was archived by the owner on Oct 19, 2023. It is now read-only.

Added grpc extension. #62

Merged
merged 2 commits into from
Mar 9, 2016
Merged

Added grpc extension. #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efeb7c6
Added grpc extension.
Feb 19, 2016
74aed41
Added an end to end test for grpc extension.
Mar 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions e2e/EndToEndTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,11 @@ public function testSession()
'session.php status code');
$this->assertEquals('1', $body = $resp->getBody()->getContents());
}

public function testGrpcPubsub()
{
$resp = $this->client->get('grpc_pubsub.php');
$this->assertEquals('200', $resp->getStatusCode(),
'grpc_pubsub.php status code');
}
}
1 change: 1 addition & 0 deletions php-nginx/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ php.ini):
These extensions are only available with PHP 5.6:

- suhosin (shared, but enabled by default)
- gRPC (shared)

## Add something to php.ini

Expand Down
4 changes: 2 additions & 2 deletions php-nginx/build-scripts/build_nginx.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ rm nginx.tar.gz.asc
pushd /usr/src/nginx
./configure \
--prefix=$NGINX_DIR \
--error-log-path=$LOG_DIR/nginx-error.log \
--http-log-path=$LOG_DIR/nginx-access.log \
--error-log-path=$LOG_DIR/app-nginx-error.log \
--http-log-path=$LOG_DIR/app-nginx-access.log \
--user=www-data \
--group=www-data \
--with-http_gzip_static_module \
Expand Down
1 change: 1 addition & 0 deletions php-nginx/build-scripts/build_php56.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ mkdir -p ${PHP56_DIR}/lib/conf.d
${PHP56_DIR}/bin/pecl install memcache
${PHP56_DIR}/bin/pecl install mongodb
${PHP56_DIR}/bin/pecl install redis
${PHP56_DIR}/bin/pecl install grpc-beta

rm -rf /tmp/pear

Expand Down
4 changes: 2 additions & 2 deletions php-nginx/nginx.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ daemon off;

user www-data;
worker_processes %%NPROC%%;
error_log /var/log/app_engine/nginx-error.log info;
error_log /var/log/app_engine/app-nginx-error.log info;

events {
worker_connections 4096;
Expand All @@ -31,7 +31,7 @@ http {

client_max_body_size 32m;

access_log /var/log/app_engine/nginx-access.log;
access_log /var/log/app_engine/app-nginx-access.log;

sendfile on;

Expand Down
4 changes: 3 additions & 1 deletion scripts/run_test_suite.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ set -ex

# Run php-cs-fixer.
# We want to fail fast for coding standard violations.
php-cs-fixer fix --dry-run --diff --level=psr2 --fixers=concat_with_spaces .
php-cs-fixer fix --dry-run --diff --level=psr2 \
--fixers=concat_with_spaces,unused_use,trailing_spaces,indentation .


# Then build images.
scripts/build_images.sh
Expand Down
4 changes: 2 additions & 2 deletions testapps/php56_custom/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"require": {
"silex/silex": "^1.3",
"php": "^5.6"
"php": "^5.6",
"silex/silex": "^1.3"
}
}
1 change: 1 addition & 0 deletions testapps/php56_custom/php.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,4 @@ extension=xmlrpc.so
extension=xsl.so
extension=mongodb.so
extension=redis.so
extension=grpc.so
16 changes: 14 additions & 2 deletions testapps/php56_e2e/composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
{
"repositories": [
{
"type": "vcs",
"url": "https://github.com/stanley-cheung/Protobuf-PHP"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is @stanley-cheung interested in submitting his changes datto/protobuf-php? Why are we using his fork?

Copy link

Choose a reason for hiding this comment

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

We tried, but the upstream repo has not responded in a year: drslump/Protobuf-PHP#48

But because he has submited his package to Packagist, we can't submit our fork unless we change the repo name.

That was why I have to resort to this composer.json syntax to use his official package on Packagist in the require section but substitute the actual content with our fork.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this get pulled in implicitly as part of the composer requirements for grpc?

Copy link

Choose a reason for hiding this comment

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

That is true. By pulling grpc/grpc, you are also implicitly pulling datto/protobuf-php (with a substituted fork) yes.

Edit: sorry, I take that back. The composer.json file you should be looking at should be at the top level of the grpc repo: https://github.com/grpc/grpc/blob/master/composer.json. Packagist only look at composer.json at the top level of a repo. There we did not pull in datto/protobuf-php. I believe at that time we thought pulling in datto/protobuf-php should be left to user of grpc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so there's no need for specifying datto/protofuf-php here. I will remove 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.

No problem, I added it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK gotcha. I advocate making it the default, since it will be very difficult for users to discover the need for them to override with your fork on their own.

Copy link

Choose a reason for hiding this comment

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

So I just tried this again yesterday. Even if the root level grpc/composer.json is going to pull in datto/protobuf-php and with my stanley-cheung override, as a user of grpc/grpc, you are not going to get the stanley-cheung override unless the user also puts that in their own composer.json. In other words, as a user of grpc/grpc, at the minimum, they still need to require grpc/grpc and add the stanley-cheung/protobuf-php override. But then at least they don't need to include datto/protobuf-php. It's not ideal but I can spell it out more in grpc's documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stanley-cheung Yes, we should add this to composer. Also, are you sure the override is required in the userland composer.json? If so, that seems like a bug in composer. And also if so, we could submit your fork as a new composer package, which would definitely fix the issue.

Requiring the users to override in composer.json rather than just call composer require grpc/grpc is not acceptable DX IMHO. We could fork to GoogleCloudPlatform and submit a new package from there.

Copy link

Choose a reason for hiding this comment

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

Yes I have tried this again and an end-user needs to do this override, and I agree that it's non-ideal. OTOH, this was supposed to be a temporary bridgegap before we have official PHP support in google/protobuf - it's being implemented now. So if we can wait for a few months, this will all go away and we will have the official solution.

}
],
"require": {
"silex/silex": "^1.3",
"php": "^5.6"
"php": "^5.6",
"datto/protobuf-php": "dev-master",
"google/auth": "dev-master",
"grpc/grpc": "dev-release-0_13",
"silex/silex": "^1.3"
},
"autoload": {
"classmap": ["web"]
}
}
Loading