wp-cli / wp-config-transformer

Programmatically edit a wp-config.php file
MIT License
80 stars 25 forks source link

Trailing whitespace messes up parse_wp_config #7

Closed bitwombat closed 6 years ago

bitwombat commented 6 years ago

If I have this as my wp-config.php

<?php
define('WP_CACHE', true); //
define('DB_NAME', 'wtngcoma_wtng');

This is what $config ends up being in WPConfigTransformer->parse_wp_config:

<?php\ndefine('WP_CACHE', true); define('DB_NAME', 'wtngcoma_wtng');

See that space before the second define? That messes up the preg_replace in update.

I can write a test and attempt to fix it, but I'm not sure where to put the test. Some guidance there and/or opinion about this bug?

frankiejarrett commented 6 years ago

@bitwombat Thanks for sharing this use case. I believe this falls under a known issue. The current implementation can only match one definition per line.

https://github.com/wp-cli/wp-config-transformer/blob/master/README.md#known-issues

bitwombat commented 6 years ago

But there is only one definition per line...

I think you're confusing my original wp-config.php input, which has one definition per line, with the $config string that parse_wp_config makes. See above.

frankiejarrett commented 6 years ago

@bitwombat ah that makes sense. What version of WP-CLI are you using? I just tried to reproduce locally with 2.0.1 and couldn't.

bitwombat commented 6 years ago

I'm using 2.0.1

Try with my original text up there verbatim. The trailing // is what causes the space.

Then:

$ wp config set DB_NAME fjarrett
Success: Updated the constant 'DB_NAME' in the 'wp-config.php' file with the value 'fjarrett'.
$ cat wp-config.php
<?php
define('WP_CACHE', true); //
define('DB_NAME', 'wtngcoma_wtng');
danielbachhuber commented 6 years ago

I can write a test and attempt to fix it, but I'm not sure where to put the test.

Take a look at https://github.com/wp-cli/wp-config-transformer/tree/master/tests for prior art on how to structure your tests. If you could write a failing test case for this in a pull request, it would be a great way to document the bug.

bitwombat commented 6 years ago

So, it looks like I should be adding to 2-UpdateTest.php

And the way it works is that there's an empty file created that then gets variables and constants added to it by the tests.

The values are from raw-data.txt and string-data.txt.

testRawConstants, testStringConstants, testRawVariables, and testStringVariables (four combinations) are all looping through raw data or string data and adding variables or constants to the test file.

The remaining tests are testing random behaviours.

Finally testConfigValues reads the created test file in and verifies all the raw data and string data test values, and the misc test's results.

All this means that all tests in 2-UpdateTest are dependent on each other, so all must be run at once.

bitwombat commented 6 years ago

Ah bloody @#(! @fjarrett's commit on master (#ad435a) fixed it back in April! Just not released yet.

Though the change wasn't intended to fix this problem, so I'll probably still write a test.

bitwombat commented 6 years ago

8

danielbachhuber commented 6 years ago

Ah bloody @#(! @fjarrett's commit on master (#ad435a) fixed it back in April! Just not released yet.

Sorry about that, @bitwombat :( I'll tag a 1.2.2 release when your test passes.

bitwombat commented 6 years ago

My test passes now because of #ad435a. So should be all set then #8 gets merged?

schlessera commented 6 years ago

Fixed in v1.2.2