-
Notifications
You must be signed in to change notification settings - Fork 1k
Preserve "0" values in YAML output #6188
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@swissspidy Thanks for the suggestion! You’re right — running tests locally first would have saved some iteration time. I’ve since been running PHPCS and the relevant Behat scenarios locally before pushing, which helped stabilize the changes quickly. All unit tests, functional tests, and Behat scenarios now pass both locally and in CI. You may notice the Codecov check reporting 0% of diff hit, this appears to be due to coverage not being uploaded for this test matrix rather than missing test coverage. The behavior is fully covered by the existing Behat scenario and passes consistently. |
This is expected. You have to wait until all tests ran before the coverage report is uploaded. You will notice now that the change to the
That's not correct, FWIW. There is no crash or exception. Here's what I am seeing: Before Notice the 3 dashes in each case. After: Notice the difference. If https://github.com/wp-cli/spyc/blob/17b68259d8d49f6a8e2922db8eb1c3463376a4e7/src/Spyc.php#L237-L238 I'd say this warrants a closer look to make sure this change is made at the right place. |
In PHP, empty(0) and empty("0") return true, which caused the YAML
dumper to incorrectly treat zero values as empty. This resulted in:
- Arrays containing 0 being serialized without the zero value
- Scalar zero values being omitted from YAML output
Changes:
- dump(): Replace 'if ($array)' with explicit null/empty checks to
properly handle arrays that may contain zero values
- _yamlize(): Replace 'empty($value)' with 'count($value) === 0' to
only treat actually empty arrays as empty, not zero values
Added test cases:
- testDumpIntegerZero: Verify integer 0 is correctly dumped
- testDumpStringZero: Verify string '0' is correctly dumped
- testDumpAssociativeZero: Verify zero values in associative arrays
- testDumpMixedWithZero: Verify mixed arrays containing zero
Fixes wp-cli/wp-cli#6188
Add a Behat test to verify that YAML output correctly preserves zero values. This test validates the fix in wp-cli/spyc (see wp-cli/spyc PR). The root cause of zero values being dropped in YAML output is in the Spyc library, where: - empty($value) incorrectly treats 0 and "0" as empty in PHP - if ($array) skips processing when the array value is falsey This PR only includes the test - the actual fix is in wp-cli/spyc. Depends on: wp-cli/spyc#XX (fix-yaml-zero-values branch) Fixes wp-cli#6188
8dfa47b to
bd41197
Compare
|
Hi @swissspidy, thanks for the detailed review! You were absolutely right — the fix belongs in Spyc, not here. I've created a PR to address the root cause: 👉 wp-cli/spyc#3 - Fix YAML dump incorrectly handling zero values The Spyc fix addresses exactly what you pointed out:
I've updated this PR to:
The test is expected to fail until Spyc is updated. Once wp-cli/spyc#3 is merged and the dependency is updated, this test should pass. Let me know if you'd like any other changes! |
Summary
When formatting output as YAML, zero values (0 or "0") are incorrectly omitted from the output.
For example:
$ wp option update test_zero "0"
$ wp option get test_zero --format=yaml
Expected output:
"0"
Actual output:
The zero value is silently dropped.
Root Cause
This issue originates in the Spyc YAML library (wp-cli/spyc), not in WP-CLI itself.
Specifically, the following checks in Spyc.php incorrectly treat zero values as empty due to PHP’s falsey semantics:
Line 214
if ( $array )
Fails when an array contains 0, because 0 is falsey in PHP.
Line 237
if ( empty( $value ) )
empty( 0 ) and empty( "0" ) both return true, causing valid scalar zero values to be skipped during YAML serialization.
What This PR Does
This PR adds a Behat test to WP-CLI that asserts zero values are preserved in YAML output.
The test documents the expected behavior and protects against regressions once the underlying Spyc issue is fixed.
This test is expected to fail until the Spyc fix is merged and WP-CLI updates its dependency.
Dependency
This PR depends on the following fix in Spyc:
wp-cli/spyc#3 – Fix YAML dump incorrectly handling zero values
Merge Order
✅ Merge wp-cli/spyc#3
🔄 Update WP-CLI’s Spyc dependency to include the fix
✅ This PR’s test will pass and can be merged