Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes a bug where YAML values containing JSON maps were not parsed correctly. The approach of using json_decode is appropriate. However, the implementation has a critical compatibility issue by using str_starts_with (a PHP 8+ function), and it could be made more robust and efficient. I've provided a detailed comment with a suggested code change to address these points, ensuring backward compatibility and improving the code's resilience.
| if ( str_starts_with( $value, '[{' ) || str_starts_with( $value, '{"' ) ) { | ||
| $decoded = json_decode( preg_replace( '/^.*?:\s*/', '', $line ), true ); | ||
| if ($decoded !== null || (json_last_error() === JSON_ERROR_NONE && $decoded === null)) { | ||
| // Accept null as valid JSON value, but only if no error occurred | ||
| $value = $decoded; | ||
| } | ||
| // Otherwise, leave $value as-is (fallback to original string) | ||
| } |
There was a problem hiding this comment.
This is a good fix for handling JSON values. However, the current implementation has a few issues that should be addressed:
- PHP Compatibility (
critical): The use ofstr_starts_with()was introduced in PHP 8.0. Using it will break backward compatibility for projects running on older PHP versions, which this library appears to support. - Redundant Parsing (
high): The code usespreg_replace('/^.*?:\s*/', '', $line)to re-extract the value from the line. This is unnecessary and less efficient, as the value has already been correctly parsed into the$valuevariable. - Brittle JSON Detection (
medium): The condition to detect a JSON string is too specific. It will fail for valid JSON with leading whitespace (e.g.,{ "key": "value" }) or other valid JSON structures.
I've provided a suggestion below that resolves all these points by using a more robust and backward-compatible approach.
// The default inline parser (_toType) does not correctly handle complex JSON objects.
// Attempt to parse as JSON first for values that look like potential JSON.
$first_char = isset($value[0]) ? $value[0] : '';
if ($first_char === '{' || $first_char === '[') {
$decoded = json_decode($value, true);
// If the value is valid JSON, use the decoded array.
// This correctly handles valid JSON 'null' as well.
if (json_last_error() === JSON_ERROR_NONE) {
$value = $decoded;
}
// Otherwise, leave $value as a string for the legacy parser to handle.
}
Valid YAML such as the following is not correctly parsed by the Spyc fork used in wp-cli. If you validate the following with other YAML validators such as yamllint.com, it is returned as valid Yaml.
This should be represented in PHP as:
However, when running through the Spyc.php implementation, the following is returned:
This can be tested by running one of the example scripts
I have added two additional tests in the
spyc.yamlfile to test this issue. If you run the exampleyaml-load.phpscript you will see that they are handled correctly and all of the existing examples are unchanged.