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

Bug: False positive result shown when theme slug is entered in non-lowercase for activation #121

Open
arunsathiya opened this Issue Aug 12, 2018 · 8 comments

Comments

5 participants
@arunsathiya

arunsathiya commented Aug 12, 2018

Steps to reproduce: Follow the steps below to reproduce the issue.

  • Enter theme slug in non-lowercase when activating it (Examples: wp theme activate Dara, `wp theme
  • Notice a success message shown Success: Switched to 'Dara' theme.
  • Enter wp theme list to notice that no themes are active
➜  local wp theme list
+-----------------+----------+--------+---------+
| name            | status   | update | version |
+-----------------+----------+--------+---------+
| dara            | inactive | none   | 1.2.1   |
| twentyseventeen | inactive | none   | 1.7     |
+-----------------+----------+--------+---------+
➜  local

What is expected: Theme activation does not go through, and the success notice is not shown.

What happens instead: A success message is shown, but no theme is not activated.

Screencast indicating this behaviour: https://asciinema.org/a/JICHDMDwcPjsVibxZMUlHOHRJ

In theory, slug is supposed to be entered in lowercase.

--

  • Tested on a local install of WordPress. WordPress 4.9.8
  • PHP version: 7.2.8
  • WP-CLI version: 2.0.0
@arunsathiya

This comment has been minimized.

arunsathiya commented Aug 12, 2018

Although, upon visiting the site, Dara theme is indeed activated.

@wojsmol

This comment has been minimized.

Contributor

wojsmol commented Aug 12, 2018

@arunsathiya Please post output of wp --info

@arunsathiya

This comment has been minimized.

arunsathiya commented Aug 12, 2018

@wojsmol Here you go.

➜  local wp --info
OS:	Darwin 17.7.0 Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64 x86_64
Shell:	/bin/zsh
PHP binary:	/usr/local/Cellar/php/7.2.8/bin/php
PHP version:	7.2.8
php.ini used:	/usr/local/etc/php/7.2/php.ini
WP-CLI root dir:	phar://wp-cli.phar/vendor/wp-cli/wp-cli
WP-CLI vendor dir:	phar://wp-cli.phar/vendor
WP_CLI phar path:	/Users/arunsathiya/sites/local
WP-CLI packages dir:	/Users/arunsathiya/.wp-cli/packages/
WP-CLI global config:
WP-CLI project config:
WP-CLI version:	2.0.0
➜  local
@schlessera

This comment has been minimized.

Member

schlessera commented Aug 23, 2018

Thanks for the report, @arunsathiya!

This is fixed in #126.

@schlessera schlessera added this to the 2.0.1 milestone Aug 23, 2018

@preda-bogdan

This comment has been minimized.

preda-bogdan commented Aug 28, 2018

@schlessera this breaks wp theme install <zip> --activate because you now assume that the unzipped folder is all lowercase witch is not always the case, also this does not reflect the same functionality as in the admin interface where you can activate themes with lower and uppercase characters as well as with spaces and other special characters.

I think the CLI should reflect the admin functionality for consistency sake.

The behavior for @arunsathiya 's case IMO should be to throw an error that the specified Uppercase theme to be activated is not installed.

@davrosait

This comment has been minimized.

davrosait commented Sep 5, 2018

@schlessera i agree with @preda-bogdan This should reflect same functionality as admin interface.
This breaks wp theme activate Avada whereas activation in admin interface succeeds.

@schlessera

This comment has been minimized.

Member

schlessera commented Sep 5, 2018

Yes, looks like I'm gonna revert this change for now.

Detecting which slugs will be valid for themes seems to be non-trivial, and Core does not take care of it as it does for plugins.

@schlessera

This comment has been minimized.

Member

schlessera commented Oct 2, 2018

#126 has been reverted, so reopening this issue for now.

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