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

Do something about wp-cli-ssh #879

Merged
merged 3 commits into from Nov 22, 2013

Conversation

3 participants
@scribu
Member

scribu commented Nov 21, 2013

Today I learned that calling require() from a class method gives the code inside the file access to $this:

https://github.com/x-team/wp-cli-ssh/blob/master/wp-cli-ssh.php

It's an amazing hack, but it's also fragile. It could easily break when WP_CLI\Runner is refactored in the future. Or, we end up like Core, where major refactorings are not possible, because one or more popular extensions would break.

@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu
Member

scribu commented Nov 21, 2013

@jonathanbardo

This comment has been minimized.

Show comment
Hide comment
@jonathanbardo

jonathanbardo Nov 21, 2013

Contributor

I'm glad we were able to use this because it gave me access to private method I couldn't have used otherwise. @scribu is right, we need a better of doing things because this could easily break.

Contributor

jonathanbardo commented Nov 21, 2013

I'm glad we were able to use this because it gave me access to private method I couldn't have used otherwise. @scribu is right, we need a better of doing things because this could easily break.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Nov 21, 2013

Contributor

It seems to me that ideally WP-CLI would provide some more hooks to allow extensions to do some lower-level things:

  • Allow the Configurator spec to be extended with new global parameters (e.g. --ssh-host) and global configs (e.g. the ssh array entry in wp-cli.yml). #520
  • Allow the config array to be mutated instead of just accessed (e.g. add a WP_CLI::set_config() method) so that we can override disabled_commands.
  • Make available an early-early invoke hook, for example right after the requires are processed.
Contributor

westonruter commented Nov 21, 2013

It seems to me that ideally WP-CLI would provide some more hooks to allow extensions to do some lower-level things:

  • Allow the Configurator spec to be extended with new global parameters (e.g. --ssh-host) and global configs (e.g. the ssh array entry in wp-cli.yml). #520
  • Allow the config array to be mutated instead of just accessed (e.g. add a WP_CLI::set_config() method) so that we can override disabled_commands.
  • Make available an early-early invoke hook, for example right after the requires are processed.
@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu

scribu Nov 21, 2013

Member

A cleaner approach would be to implement it as a completely separate script:

php wp-cli-ssh.php staging plugin status

where wp-cli-ssh.php would look something like this:

<?php

require __DIR__ . '/vendor/autoload.php';

$info = json_decode( exec( 'wp cli info --format=json' ) );

use Symfony\Component\Yaml\Yaml;

$global_config = Yaml::parse( $info['global_config_path'] );

...

wp cli info --format=json needs to be implemented.

Member

scribu commented Nov 21, 2013

A cleaner approach would be to implement it as a completely separate script:

php wp-cli-ssh.php staging plugin status

where wp-cli-ssh.php would look something like this:

<?php

require __DIR__ . '/vendor/autoload.php';

$info = json_decode( exec( 'wp cli info --format=json' ) );

use Symfony\Component\Yaml\Yaml;

$global_config = Yaml::parse( $info['global_config_path'] );

...

wp cli info --format=json needs to be implemented.

@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu

scribu Nov 21, 2013

Member

/vendor/autoload.php would be a separate autoload, just for wp-cli-ssh, and not the one from the WP-CLI install.

Member

scribu commented Nov 21, 2013

/vendor/autoload.php would be a separate autoload, just for wp-cli-ssh, and not the one from the WP-CLI install.

@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu

scribu Nov 21, 2013

Member

Yes, this would mean that wp-cli-ssh won't be displayed on http://wp-cli.org/package-index/, but I'm sure we can find some other way to promote it.

Member

scribu commented Nov 21, 2013

Yes, this would mean that wp-cli-ssh won't be displayed on http://wp-cli.org/package-index/, but I'm sure we can find some other way to promote it.

@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu

scribu Nov 21, 2013

Member

I've added some commits. Let me know if there's any other thing WP-CLI could do to make it easier to implement wp-cli-ssh as a separate utilty.

Member

scribu commented Nov 21, 2013

I've added some commits. Let me know if there's any other thing WP-CLI could do to make it easier to implement wp-cli-ssh as a separate utilty.

@jonathanbardo

This comment has been minimized.

Show comment
Hide comment
@jonathanbardo

jonathanbardo Nov 21, 2013

Contributor

@scribu At first, I liked the fact of using wp-cli to send request to remote servers like vagrant.

Now that I think about it, having a separate utility would imply we don't even need to install wp-cli on local anymore (because it's installed part of vvv for example) and would only need the PHP install.

What do you think @westonruter ?

Contributor

jonathanbardo commented Nov 21, 2013

@scribu At first, I liked the fact of using wp-cli to send request to remote servers like vagrant.

Now that I think about it, having a separate utility would imply we don't even need to install wp-cli on local anymore (because it's installed part of vvv for example) and would only need the PHP install.

What do you think @westonruter ?

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Nov 22, 2013

Contributor

As discussed with @jonathanbardo, we're going to see if we can refactor the logic to add an ssh command, with wildcarded subcommands which then get forwarded to the remote server. xwp/wp-cli-ssh#4

Contributor

westonruter commented Nov 22, 2013

As discussed with @jonathanbardo, we're going to see if we can refactor the logic to add an ssh command, with wildcarded subcommands which then get forwarded to the remote server. xwp/wp-cli-ssh#4

@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu

scribu Nov 22, 2013

Member

Ok, cool.

Member

scribu commented Nov 22, 2013

Ok, cool.

scribu pushed a commit that referenced this pull request Nov 22, 2013

Cristi Burcă
Merge pull request #879 from wp-cli/wp-cli-ssh
Do something about wp-cli-ssh

@scribu scribu merged commit 72e0689 into master Nov 22, 2013

1 check passed

default The Travis CI build passed
Details

@scribu scribu deleted the wp-cli-ssh branch Nov 22, 2013

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Nov 23, 2013

Contributor

@jonathanbardo has rewritten the command to operate by registering an ssh command: https://github.com/x-team/wp-cli-ssh/blob/master/wp-cli-ssh.php

Note that he's dome some magic with the Reflection API. We should eliminate the need for that. Most should be resolved by #520, except for find_command_to_run() and overriding the config-loaded disabled_commands.

Contributor

westonruter commented Nov 23, 2013

@jonathanbardo has rewritten the command to operate by registering an ssh command: https://github.com/x-team/wp-cli-ssh/blob/master/wp-cli-ssh.php

Note that he's dome some magic with the Reflection API. We should eliminate the need for that. Most should be resolved by #520, except for find_command_to_run() and overriding the config-loaded disabled_commands.

@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu

scribu Nov 23, 2013

Member

I don't think using find_command_to_run() on the local WP-CLI install makes sense at all, since you can have different configs on the remote server. You could just let the remote WP-CLI fail if the command is not found there.

Member

scribu commented Nov 23, 2013

I don't think using find_command_to_run() on the local WP-CLI install makes sense at all, since you can have different configs on the remote server. You could just let the remote WP-CLI fail if the command is not found there.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Nov 23, 2013

Contributor

Yeah, that's true. It is useful though for checking against the overridden disabled_commands, so the server never even is pinged if you're doing something forbidden. But in that case, the invoked command could be manually checked against the disabled_commands provided in the ssh config.

Contributor

westonruter commented Nov 23, 2013

Yeah, that's true. It is useful though for checking against the overridden disabled_commands, so the server never even is pinged if you're doing something forbidden. But in that case, the invoked command could be manually checked against the disabled_commands provided in the ssh config.

@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu

scribu Nov 23, 2013

Member

As for disabled_commands, it shouldn't be that hard to check the positional parameters manually, instead of relying on WP_CLI\Runner, especially since the positional parameters are already separated from the associative ones.

Member

scribu commented Nov 23, 2013

As for disabled_commands, it shouldn't be that hard to check the positional parameters manually, instead of relying on WP_CLI\Runner, especially since the positional parameters are already separated from the associative ones.

@scribu

This comment has been minimized.

Show comment
Hide comment
@scribu

scribu Nov 23, 2013

Member

Heh... we both wrote the same thing.

Member

scribu commented Nov 23, 2013

Heh... we both wrote the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment