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

Transform spaces to underscores when scaffolding a child theme #40

Merged
merged 3 commits into from Aug 22, 2017

Conversation

3 participants
@eliseferguson
Contributor

eliseferguson commented Aug 21, 2017

…g a child theme

@danielbachhuber

Thanks for the pull request @eliseferguson

Out of curiosity, why'd you use preg_replace() here when str_replace() also accepts an array as its first argument?

Also, it looks like you may have used spaces for indentation instead of tabs. Can you fix?

Lastly, can you include a test case reflecting this change?

@eliseferguson

This comment has been minimized.

Show comment
Hide comment
@eliseferguson

eliseferguson Aug 21, 2017

Contributor

I was thinking preg_replace to use regex in case there was more than one space (though perhaps that is moot being this is a directory name).

Contributor

eliseferguson commented Aug 21, 2017

I was thinking preg_replace to use regex in case there was more than one space (though perhaps that is moot being this is a directory name).

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 21, 2017

Member

I was thinking preg_replace to use regex in case there was more than one space

str_replace() will handle multiple spaces. Generally, str_replace() should be used over preg_replace() (when you can) because the former is more performant than the latter.

Member

danielbachhuber commented Aug 21, 2017

I was thinking preg_replace to use regex in case there was more than one space

str_replace() will handle multiple spaces. Generally, str_replace() should be used over preg_replace() (when you can) because the former is more performant than the latter.

@eliseferguson

This comment has been minimized.

Show comment
Hide comment
@eliseferguson

eliseferguson Aug 21, 2017

Contributor

I will update accordingly.

Contributor

eliseferguson commented Aug 21, 2017

I will update accordingly.

@eliseferguson

This comment has been minimized.

Show comment
Hide comment
@eliseferguson

eliseferguson Aug 21, 2017

Contributor

@danielbachhuber I'm looking at this test case stuff and I'm wondering how you'd like me to handle this being it's not really standard? I don't think there's any wp commands I can use to create a theme with a space in the directory.

Contributor

eliseferguson commented Aug 21, 2017

@danielbachhuber I'm looking at this test case stuff and I'm wondering how you'd like me to handle this being it's not really standard? I don't think there's any wp commands I can use to create a theme with a space in the directory.

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 21, 2017

Member

I don't think there's any wp commands I can use to create a theme with a space in the directory.

You could use mv:

When I run `wp theme install p2 && mv wp-content/themes/p2 'wp-content/themes/p2 with space'`
Then the 'wp-content/themes/p2 with space' directory should exist
Member

danielbachhuber commented Aug 21, 2017

I don't think there's any wp commands I can use to create a theme with a space in the directory.

You could use mv:

When I run `wp theme install p2 && mv wp-content/themes/p2 'wp-content/themes/p2 with space'`
Then the 'wp-content/themes/p2 with space' directory should exist
@eliseferguson

This comment has been minimized.

Show comment
Hide comment
@eliseferguson

eliseferguson Aug 22, 2017

Contributor

Okay so here's an attempt at a test case:

Background:
Given a WP install
When I run wp theme install p2 && mv wp-content/themes/p2 wp-content/themes/p2\ with\ space
Then the 'wp-content/themes/p2 with space' directory should exist

When I run `wp theme path`
Then save STDOUT as {THEME_DIR}

Scenario: Scaffold theme tests parent theme with space in directory
When I run wp scaffold child-theme p2-child-theme --parent_theme='p2 with space'

Then the {THEME_DIR}/p2 with space/tests/functions.php file should contain:
      """
      p2_with_space_parent_theme_enqueue_styles()
      """
Contributor

eliseferguson commented Aug 22, 2017

Okay so here's an attempt at a test case:

Background:
Given a WP install
When I run wp theme install p2 && mv wp-content/themes/p2 wp-content/themes/p2\ with\ space
Then the 'wp-content/themes/p2 with space' directory should exist

When I run `wp theme path`
Then save STDOUT as {THEME_DIR}

Scenario: Scaffold theme tests parent theme with space in directory
When I run wp scaffold child-theme p2-child-theme --parent_theme='p2 with space'

Then the {THEME_DIR}/p2 with space/tests/functions.php file should contain:
      """
      p2_with_space_parent_theme_enqueue_styles()
      """

@danielbachhuber danielbachhuber added this to the 1.0.6 milestone Aug 22, 2017

@danielbachhuber danielbachhuber changed the title from Update str_replace to preg_replace and to include spaces when creatin… to Transform spaces to underscores when scaffolding a child theme Aug 22, 2017

@miya0001

@danielbachhuber @eliseferguson

I can understand we should use str_replace() than preg_replace, but in this case I feel preg_replace is better, because we should replace all chars which are not allowed to use as a name of the function like following.

preg_replace( '/\W/', '_', $data['parent_theme'] )
@@ -372,8 +372,7 @@ function child_theme( $args, $assoc_args ) {
'theme_uri' => ""
) );
$data['slug'] = $theme_slug;
$data['parent_theme_function_safe'] = str_replace( '-', '_', $data['parent_theme'] );
$data['parent_theme_function_safe'] = str_replace( array( ' ', '-' ), '_', $data['parent_theme'] );

This comment has been minimized.

@miya0001

miya0001 Aug 22, 2017

Member

Let's replace all non-word characters.

$data['parent_theme_function_safe'] = preg_replace( '/\W/', '_', $data['parent_theme'] ) ;
@miya0001

miya0001 Aug 22, 2017

Member

Let's replace all non-word characters.

$data['parent_theme_function_safe'] = preg_replace( '/\W/', '_', $data['parent_theme'] ) ;

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 22, 2017

Member

@miya0001 Feel free to open a new issue on the topic.

@danielbachhuber

danielbachhuber Aug 22, 2017

Member

@miya0001 Feel free to open a new issue on the topic.

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 22, 2017

Member

@miya0001 To clarify further, I think this pull request is fine because it fixes the immediate issue. It's also a more specific change than using preg_replace( '/\W/', which could have unintended consequences.

@danielbachhuber

danielbachhuber Aug 22, 2017

Member

@miya0001 To clarify further, I think this pull request is fine because it fixes the immediate issue. It's also a more specific change than using preg_replace( '/\W/', which could have unintended consequences.

This comment has been minimized.

@miya0001

miya0001 Aug 22, 2017

Member

OK. 😄

@miya0001

miya0001 Aug 22, 2017

Member

OK. 😄

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 22, 2017

Member

Okay so here's an attempt at a test case:

Looks great!

I'm going to land this without tests. It's a minor enough change, and I suspect you'll discover there are problems in the test suite when using spaces in paths.

Member

danielbachhuber commented Aug 22, 2017

Okay so here's an attempt at a test case:

Looks great!

I'm going to land this without tests. It's a minor enough change, and I suspect you'll discover there are problems in the test suite when using spaces in paths.

@danielbachhuber danielbachhuber merged commit decd726 into wp-cli:master Aug 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment