Skip to content

Use correct default $WP_TESTS_DIR on MacOS#67

Merged
gitlost merged 2 commits intomasterfrom
improve-bootstrap
Oct 15, 2017
Merged

Use correct default $WP_TESTS_DIR on MacOS#67
gitlost merged 2 commits intomasterfrom
improve-bootstrap

Conversation

@miya0001
Copy link
Copy Markdown
Member

@miya0001 miya0001 commented Oct 14, 2017

The default value of $TMPDIR is not /tmp on macOS.

Related: #61

@miya0001 miya0001 changed the title get correct WP_TESTS_DIR get correct WP_TESTS_DIR on mcOS by default Oct 14, 2017
@miya0001 miya0001 requested a review from a team October 14, 2017 11:06
gitlost
gitlost suggested changes Oct 15, 2017
View reviewed changes
templates/plugin-bootstrap.mustache Outdated

if ( ! $_tests_dir ) {
$_tests_dir = '/tmp/wordpress-tests-lib';
$_tmpdir = getenv( 'TMPDIR' );
Copy link
Copy Markdown
Contributor

@gitlost gitlost Oct 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

For Windows compat could use $_tmpdir = sys_get_temp_dir() instead?

gitlost
gitlost suggested changes Oct 15, 2017
View reviewed changes
templates/plugin-bootstrap.mustache Outdated
if ( ! $_tmpdir ) {
$_tmpdir = '/tmp';
}
$_tests_dir = preg_replace( '#/$#', '', $_tmpdir ) . '/wordpress-tests-lib';
Copy link
Copy Markdown
Contributor

@gitlost gitlost Oct 15, 2017

Choose a reason for hiding this comment

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

Similarly rtrim( $_tmpdir, '/\\' ) instead of the preg_replace()?

@gitlost
Copy link
Copy Markdown
Contributor

gitlost commented Oct 15, 2017

Actually noticed that sys_get_temp_dir() will always return something (/tmp as last resort) https://github.com/php/php-src/blob/php-5.3.0/main/php_open_temporary_file.c#L215 so this code and WP_CLI\Utils\get_temp_dir() could be simplified.

@miya0001
Copy link
Copy Markdown
Member Author

miya0001 commented Oct 15, 2017

@gitlost Thanks 😊

I have a basic question. What is \\ meaning?
Why it should be rtrim( $_tmpdir, '/\\' ) instead of the rtrim( $_tmpdir, '/' )?

Actually noticed that sys_get_temp_dir() will always return something (/tmp as last resort) https://github.com/php/php-src/blob/php-5.3.0/main/php_open_temporary_file.c#L215 so this code and WP_CLI\Utils\get_temp_dir() could be simplified.

Oh!

@gitlost
Copy link
Copy Markdown
Contributor

gitlost commented Oct 15, 2017

I have a basic question. What is \\ meaning?

It's just a backslash, the Windows directory separator. In single-quoted PHP strings, you have to escape a backslash (and a single quote) with a backslash (language.types.string.syntax.single).

sys_get_temp_dir() will always return something

So this code could be simplified to $_tests_dir = rtrim( sys_get_temp_dir(), '/\\' ) . '/wordpress-tests-lib';

@miya0001
Copy link
Copy Markdown
Member Author

miya0001 commented Oct 15, 2017

the Windows directory separator

!! 👍

@miya0001
Copy link
Copy Markdown
Member Author

miya0001 commented Oct 15, 2017

@gitlost Thanks! I did. 😊

@gitlost gitlost added bug command:scaffold-plugin Related to 'scaffold plugin' command labels Oct 15, 2017
@gitlost gitlost added this to the 1.0.10 milestone Oct 15, 2017
@gitlost gitlost merged commit e0655ae into master Oct 15, 2017
@gitlost gitlost deleted the improve-bootstrap branch October 15, 2017 13:16
@danielbachhuber danielbachhuber added command:scaffold-plugin-tests Related to 'scaffold plugin-tests' command and removed command:scaffold-plugin Related to 'scaffold plugin' command labels Oct 16, 2017
@danielbachhuber danielbachhuber changed the title get correct WP_TESTS_DIR on mcOS by default Use correct default $WP_TESTS_DIR on MacOS Oct 16, 2017
@torounit torounit mentioned this pull request Mar 1, 2018
Merged
danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022
@gitlost
Merge pull request #67 from wp-cli/improve-bootstrap
fe5f7cc 
get correct WP_TESTS_DIR on mcOS by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gitlost gitlost gitlost approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug command:scaffold-plugin-tests Related to 'scaffold plugin-tests' command

Projects

None yet

Milestone

1.0.10

Development

Successfully merging this pull request may close these issues.

3 participants

@miya0001 @gitlost @danielbachhuber