Prevent trailing commas in define() from being included in the extracted value

[SteenSchutt]

The current regex does not support trailing commas in the define construct (To the point where it goes completely bonkers). I'd like to propose a minor change that will address this problem.

Using the following code:

<?php
require __DIR__ . "/vendor/autoload.php";
$c = new WPConfigTransformer(__DIR__ . '/wp-config.php');
foreach([ 'DB_NAME', 'DB_USER', 'DB_PASSWORD', 'DB_HOST' ] as $var) {
  var_dump($c->get_value('constant', $var));
}

wp-config.php is the example used in README.MD, but with a few modifications, specifically adding trailing commas with and without the third parameter:

define('DB_NAME', 'database_name_here',);
define('DB_HOST', 'localhost', true, );

The full example is provided here: https://gist.github.com/SteenSchutt/ca4413bf3aad75f9eb57f2b2e2999004

Output with current version:

WARNING  Undefined array key "DB_USER" in src/WPConfigTransformer.php on line 108.
WARNING  Trying to access array offset on value of type null in src/WPConfigTransformer.php on line 108.

string(59) "'database_name_here',);

define('DB_USER', 'username_here'"
NULL
string(15) "'password_here'"
string(51) "'localhost', true, );

define('DB_CHARSET', 'utf8'"

Output with my changes:

string(20) "'database_name_here'"
string(15) "'username_here'"
string(15) "'password_here'"
string(11) "'localhost'"

I tried getting the tests to run so I could add to those as well, but I continuously ran my head against a wall, so I hope you can help by adding some fixtures/tests that can check this. As far as I can tell, everything else still matches as before.

The number of steps required to preg_match the config file appears to be basically the same as before as well.

Thank you for providing this class as a standalone package, it has been massively helpful :)

[SteenSchutt]

Looks like the tests are good. Just wanted to add that this is what the matches with the 1.3.3 regex look like in regex101:

[danielbachhuber]

I tried getting the tests to run so I could add to those as well, but I continuously ran my head against a wall, so I hope you can help by adding some fixtures/tests that can check this.

@SteenSchutt Out of curiosity, what problems did you run into?

[SteenSchutt]

The README just says to run phpunit, but I'm getting some namespace errors. I don't have much experience using phpunit, so I'm not entirely sure where these things are supposed to come from.

I'm using this Dockerfile:

FROM php:8.2-cli
COPY --from=composer /usr/bin/composer /usr/bin/composer
COPY ./ /data
WORKDIR /data
RUN composer install
ENTRYPOINT /bin/bash

Getting the following output:

root@7a5f28489ae4:/data# ./vendor/bin/phpunit 

Fatal error: Uncaught Error: Class "WP_CLI\Tests\TestCase" not found in /data/tests/0-SetupTest.php:5
Stack trace:
#0 /data/vendor/phpunit/phpunit/src/Util/FileLoader.php(66): include_once()
#1 /data/vendor/phpunit/phpunit/src/Util/FileLoader.php(49): PHPUnit\Util\FileLoader::load('/data/tests/0-S...')
#2 /data/vendor/phpunit/phpunit/src/Framework/TestSuite.php(397): PHPUnit\Util\FileLoader::checkAndLoad('/data/tests/0-S...')
#3 /data/vendor/phpunit/phpunit/src/Framework/TestSuite.php(536): PHPUnit\Framework\TestSuite->addTestFile('/data/tests/0-S...')
#4 /data/vendor/phpunit/phpunit/src/TextUI/TestSuiteMapper.php(67): PHPUnit\Framework\TestSuite->addTestFiles(Array)
#5 /data/vendor/phpunit/phpunit/src/TextUI/Command.php(389): PHPUnit\TextUI\TestSuiteMapper->map(Object(PHPUnit\TextUI\XmlConfiguration\TestSuiteCollection), '')
#6 /data/vendor/phpunit/phpunit/src/TextUI/Command.php(112): PHPUnit\TextUI\Command->handleArguments(Array)
#7 /data/vendor/phpunit/phpunit/src/TextUI/Command.php(97): PHPUnit\TextUI\Command->run(Array, true)
#8 /data/vendor/phpunit/phpunit/phpunit(107): PHPUnit\TextUI\Command::main()
#9 /data/vendor/bin/phpunit(123): include('/data/vendor/ph...')
#10 {main}

Next PHPUnit\TextUI\RuntimeException: Class "WP_CLI\Tests\TestCase" not found in /data/vendor/phpunit/phpunit/src/TextUI/Command.php:99
Stack trace:
#0 /data/vendor/phpunit/phpunit/phpunit(107): PHPUnit\TextUI\Command::main()
#1 /data/vendor/bin/phpunit(123): include('/data/vendor/ph...')
#2 {main}
  thrown in /data/vendor/phpunit/phpunit/src/TextUI/Command.php on line 99

I am just using the PHPUnit 9.x specified in composer.json. Assuming that one should work?

[danielbachhuber]

@SteenSchutt Maybe composer install didn't work properly?

Try using composer phpunit instead of ./vendor/bin/phpunit.

[SteenSchutt]

Ah yes. composer phpunit seems to work, but using a globally installed phpunit and the one in vendor/bin didn't :+1:

[SteenSchutt]

Oh boy, I forgot how recent that PHP change allowing for trailing commas was. Any ideas on how to handle this? I can at least say that the test fails without my change, but pass with, assuming that PHP is happy with the syntax.

[danielbachhuber]

Any ideas on how to handle this?

Couple of options:

1. If it's not too difficult, set up a standalone integration test for the change and use $this->markTestSkipped() when the PHP version is insufficient.
2. Abstract the regex to a dedicated method, write unit tests for the regex, and again use $this->markTestSkipped() for the trailing comma tests.

[swissspidy]

Please let us know if you need help with writing those tests. Would definitely be a nice enhancement.

[SteenSchutt]

Haven't had the time to take a proper look at the tests to do it right (vacations and deadlines :+1:). If someone else wants to do it, that'd be great.

IIRC my main problem was trying to figure out the test architecture, and whether it would make more sense to create a new fixtures file, or whether they should just be written in the tests directly. (e.g. whether you want a fixtures/trailing-comma.txt or just want a test case with some hard coded values, and whether to use one of the existing test classes, or create a new one).