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

Prompting doesn't properly set config values #805

Merged
merged 8 commits into from Nov 22, 2013

Conversation

3 participants
@danielbachhuber
Copy link
Member

danielbachhuber commented Nov 20, 2013

I did:

salty-wordpress ➜  ftp-test.dev  wp core install --prompt
1/5 --url=<url>: ftp-test.dev
2/5 --title=<site-title>: FTP Test
3/5 --admin_user=<username>: daniel
4/5 --admin_password=<password>: daniel
5/5 --admin_email=<email>: d@danielbachhuber
Success: WordPress installed successfully.

I expected the site URL to be set to ftp-test.dev. It was instead set to example.com

@ArnaudBan

This comment has been minimized.

Copy link

ArnaudBan commented Nov 16, 2013

Same for me.

@ghost ghost assigned danielbachhuber Nov 20, 2013

@danielbachhuber

This comment has been minimized.

Copy link
Member

danielbachhuber commented Nov 20, 2013

I get this now:

salty-wordpress ➜  wordpress-trunk.dev  wp core install --prompt
Error: Parameter errors:
 missing --url parameter
 missing --title parameter
 missing --admin_user parameter
 missing --admin_password parameter
 missing --admin_email parameter

Seems prompting is gone altogether :(

@danielbachhuber

This comment has been minimized.

Copy link
Member

danielbachhuber commented Nov 20, 2013

The second bug is caused by 19fb920#diff-d5c3b8c65ee5ccc5dd15286817c05a2cR374. Because --prompt isn't something that can be specified in config.yml, it gets dropped altogether in merge_config()

danielbachhuber added some commits Nov 20, 2013

Restore the use of `--prompt` as a runtime argument.
The argument is unique of it's kind, and there isn't a great way of doing this otherwise.
@danielbachhuber

This comment has been minimized.

Copy link
Member

danielbachhuber commented Nov 20, 2013

There's actually not an easy way to support prompting for runtime arguments for the following reasons:

  • Prompting should be a seamless experience for runtime arguments & subcommand arguments.
  • The subcommand arguments (which may include a runtime argument) aren't known until the subcommand is loaded.
  • WP-CLI doesn't know which subcommand to load until it's completed init_config(), which sets and uses the runtime arguments.

However in the case of wp core install, which may be the only subcommand that uses a global config, the url value is set by wp_guess_url(). 93cb822, while ugly, does the trick just fine.

Open to alternatives.

@scribu

This comment has been minimized.

Copy link
Member

scribu commented Nov 21, 2013

We should add some Behat tests for this. It's a little awkward to test interactive commands, but it's doable (see shell.feature).

@danielbachhuber

This comment has been minimized.

Copy link
Member

danielbachhuber commented Nov 21, 2013

Happy to add tests. Can I take that as you're fine with the approach generally?

@scribu

This comment has been minimized.

Copy link
Member

scribu commented Nov 21, 2013

I don't see any major drawbacks, and I don't have any better ideas, so yeah.

@scribu

This comment has been minimized.

Copy link
Member

scribu commented Nov 21, 2013

It would be nice to expose a single, neat public method, like WP_CLI::set_url(), instead of the two private ones on the runner.

@danielbachhuber

This comment has been minimized.

Copy link
Member

danielbachhuber commented Nov 22, 2013

@scribu Ready to go... although I noticed another command that uses a runtime arg: wp core download --path=<path>. This PR doesn't solve it, and that we have a second instance tells me we need an abstracted approach for resetting runtime args via prompt. I think we should go with this PR as it exists though, because it fixes prompting which is currently broken.

It's a little awkward to test interactive commands, but it's doable

Wasn't too bad :)

self::set_url_params( $url_parts );
}

static function parse_url( $url ) {

This comment has been minimized.

@scribu

scribu Nov 22, 2013

Member

Actually, this would belong in utils.php.

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

Cristi Burcă
Merge pull request #805 from wp-cli/fix-global-prompt-805
Prompting doesn't properly set config values

@scribu scribu merged commit 2fc055b into master Nov 22, 2013

1 check passed

default The Travis CI build passed
Details

@scribu scribu deleted the fix-global-prompt-805 branch Nov 22, 2013

@scribu

This comment has been minimized.

Copy link
Member

scribu commented on 855a989 Nov 23, 2013

Looking at this again, it shouldn't be necessary, since it's mentioned in config-spec.php: https://github.com/wp-cli/wp-cli/blob/master/php/config-spec.php#L59-64

This comment has been minimized.

Copy link
Member

scribu replied Nov 23, 2013

In other words, this fixes the symptom, but the disease is actually in Configurator::merge_config(), which only checks 'file'.

This comment has been minimized.

Copy link
Member

scribu replied Nov 23, 2013

This comment has been minimized.

Copy link
Member

danielbachhuber replied Nov 23, 2013

Oh, I assumed it was intentional to only check file. My bad.

This comment has been minimized.

Copy link
Member

scribu replied Nov 23, 2013

That's alright. merge_config() was a pretty ambiguous name anyway.

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