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

Jump to bottom

add validation for slug of the plugin/theme #3666

Merged
merged 7 commits into from Dec 15, 2016
Merged

add validation for slug of the plugin/theme #3666

merged 7 commits into from Dec 15, 2016

Conversation

miya0001
Copy link
Member

Test files will be generated under the wp-content/themes when I made a mistake and ran a command like following.

$ cd wp-content/themes/twentyseventeen/
$ wp scaffold theme-tests .
Success: Created test files.

Then:

wp-content/themes
├── .travis.yml
├── bin
├── index.php
├── phpcs.ruleset.xml
├── phpunit.xml.dist
├── tests
├── twentyfifteen
├── twentyseventeen

Slug of the plugin/theme should be validated.

@miya0001 miya0001 changed the title add validation for plugin/theme add validation for slug of the plugin/theme Dec 14, 2016
danielbachhuber
danielbachhuber reviewed Dec 14, 2016
View reviewed changes
php/commands/scaffold.php
@@ -648,6 +648,9 @@ private function scaffold_plugin_theme_tests( $args, $assoc_args, $type ) {

if ( ! empty( $args[0] ) ) {
$slug = $args[0];
if ( ! preg_match( "/^[a-zA-Z0-9\-_]+$/", $slug ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you get this regex from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is from my six sense ...
I'll investigate it! 😄

@danielbachhuber
Copy link
Member

I'm not sure validating the directory name with regex is the correct direction to take, because it could have unintended consequences.

Could we instead solely ensure the target directory isn't wp-content/themes or wp-content/plugins? Also, can you add a test for this?

@miya0001
Copy link
Member Author

Did it!

@miya0001
Copy link
Member Author

The second commit allows us to run wp scaffold plugin-tests ../...
So I updated to reject . and /.

@danielbachhuber danielbachhuber added this to the 1.1.0 milestone Dec 14, 2016
danielbachhuber
danielbachhuber reviewed Dec 14, 2016
View reviewed changes
php/commands/scaffold.php
@@ -648,6 +648,9 @@ private function scaffold_plugin_theme_tests( $args, $assoc_args, $type ) {

if ( ! empty( $args[0] ) ) {
$slug = $args[0];
if ( preg_match( "#\.|\/#", $slug ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For precision, could we make this an explicit string check?

if ( in_array( $slug, array( '.' ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Yes!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, It allows like ../../../../...

@szepeviktor
Copy link
Contributor

szepeviktor commented Dec 14, 2016 •
edited

I'm not sure validating the directory name with regex is the correct direction to take, because it could have unintended consequences.

"Avoid using numbers for the theme name, as this prevents it from being displayed in the available themes list."
https://codex.wordpress.org/Theme_Development

I think the slug should be [a-z][a-z-]*[a-z] see https://wordpress.org/themes/

*Some brave guys Do Upper-Case theme slugs. We should not.

@danielbachhuber
Copy link
Member

*Some brave guys Do Upper-Case theme slugs. We should not.

I don't want to break anyone's existing usage though. We should only fix the immediate problem we're solving: . and other shorthand.

@szepeviktor
Copy link
Contributor

szepeviktor commented Dec 14, 2016 •
edited

solving: . and other shorthand.

dirname( realpath( $new_theme_path ) ) === 'themes' maybe?

@miya0001
Copy link
Member Author

miya0001 commented Dec 15, 2016 •
edited

Finally, I found same problems on wp scaffold plugin, wp scaffold child_theme and wp scaffold _s.
They will be fixed.

@miya0001
Copy link
Member Author

miya0001 commented Dec 15, 2016 •
edited

Additional note:

  • I want to allow a slug like example.com, because I sometimes use the domain name for my client work.
  • It allows us to be only in wp-content/plugins or wp-content/themes if --dir option isn't used.
  • To use . and .. for slug will always be rejected.

@danielbachhuber danielbachhuber merged commit a178823 into wp-cli:master Dec 15, 2016
@miya0001 miya0001 deleted the add-validation-for-slug branch December 30, 2016 18:57
@danielbachhuber danielbachhuber mentioned this pull request Mar 28, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielbachhuber danielbachhuber danielbachhuber left review comments

Assignees
No one assigned
Labels
command:scaffold command:scaffold-plugin-tests command:scaffold-theme-tests
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@miya0001 @danielbachhuber @szepeviktor