-
Notifications
You must be signed in to change notification settings - Fork 21
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
Moves wp-gae tool into google/cloud-tools #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a couple minor points, but overall this looks fine. Per side chat, I think a composer create-project
approach might be less code and lower friction, but that's for another day.
*/ | ||
|
||
if (file_exists(__DIR__ . '/../../../vendor/autoload.php')) { | ||
// Run the command from the git clone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow what you're getting at with these variations. The difference between a git clone and a composer install? Why not make it a composer global install?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanting to handle both the dev case (git clone
) and the vendor case (composer install
).
As for global
install, I haven't decided if that's how I want to surface these tools. That won't affect these lines, though.
src/Utils/WordPress/files/php.ini
Outdated
@@ -0,0 +1,9 @@ | |||
google_app_engine.enable_functions = "php_sapi_name, gc_enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen these google_app_engine directives, is there documentation to which this could link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these may be relevant for php55
only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing in the ini about these values. It's possible these are carried over from the php55
version of this tutorial. I'll verify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, there's nothing in the php72 docs about it either, but the php55 docs have these values. So I will remove them and verify.
Good catch!
No description provided.