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 on guestbook example is setting its include_path and using short tags. #18640

Closed
carlosrdrz opened this issue Dec 14, 2015 · 4 comments
Closed
Assignees
Labels
area/example priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@carlosrdrz
Copy link

We've found these issues after trying to run the guestbook example with a different set of containers. The issues are located at the file kubernetes/examples/guestbook/php-redis/guestbook.php, on the following lines:

<?

set_include_path('.:/usr/local/lib/php');

Although the Guestbook example works on the container referenced in the example (gcr.io/google_samples/gb-frontend:v3), it exposes some bad practices that binds that code to the specific configuration of that particular container. If you try to run the same code with a different configuration, it will fail because of that.

First, it is using PHP short tags, which won't work if the short_tags is disabled on PHP's configuration. It should use <?php instead, which is the recommended approach. Some references at http://www.php-fig.org/psr/psr-1/ or http://pear.php.net/manual/en/standards.tags.php

Also, it sets a new include_path which binds the code to a specific directory configuration, and it's not required by default. Its default value will point to the PEAR packages directory, so it should be left untouched and everything should continue to work.

@bgrant0607
Copy link
Member

cc @jeffmendoza @amygdala

@bshaffer
Copy link

+1

Thanks for providing such good documentation and support for your suggestions!

k8s-github-robot pushed a commit that referenced this issue Feb 17, 2016
(Created new GCR image version with the changes).
@jeffmendoza
Copy link
Contributor

@carlosrdrz Looks like #21327 was merged, can this be closed?

@carlosrdrz
Copy link
Author

Yep, looks like it has been solved!
Thanks for working on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/example priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

No branches or pull requests

6 participants