Skip to content
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

Fix json_encode result on DOMDocument #11840

Closed
wants to merge 2 commits into from
Closed

Conversation

nielsdos
Copy link
Member

According to https://www.php.net/manual/en/class.domdocument:
When using json_encode() on a DOMDocument object the result will be
that of encoding an empty object.

But this was broken in 8.1. The output was {"config": null}. That's because the config property is defined with a default value of NULL, hence it was included. The other properties are not included because they don't have a default property, and nothing is ever written to their backing field. Hence, the JSON encoder excludes them. Similarly, (array) $doc would yield the same config key in the array.

According to https://www.php.net/manual/en/class.domdocument:
  When using json_encode() on a DOMDocument object the result will be
  that of encoding an empty object.

But this was broken in 8.1. The output was `{"config": null}`.
That's because the config property is defined with a default value of
NULL, hence it was included. The other properties are not included
because they don't have a default property, and nothing is ever written
to their backing field. Hence, the JSON encoder excludes them.
Similarly, `(array) $doc` would yield the same `config` key in the
array.
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I suppose this might be a BC in that if someone access the value without loading a document it will now throw an Error. But then I don't see how this is likely...

@nielsdos
Copy link
Member Author

nielsdos commented Aug 1, 2023

I suppose this might be a BC in that if someone access the value without loading a document it will now throw an Error.

No it won't, with this PR:

<?php
$doc = new DOMDocument;
var_dump($doc->config);
echo json_encode($doc), "\n";

outputs:

NULL
{}

DOM properties always go through getters & setters, their backing storage is unused.
Reflection could be affected though, i.e. ReflectionProperty::hasDefaultValue() will now return false instead of true (but ReflectionProperty::getDefaultValue() will still return NULL).

@Girgias
Copy link
Member

Girgias commented Aug 1, 2023

Right, then LGTM

@nielsdos nielsdos closed this in 6e468bb Aug 1, 2023
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023
According to https://www.php.net/manual/en/class.domdocument:
  When using json_encode() on a DOMDocument object the result will be
  that of encoding an empty object.

But this was broken in 8.1. The output was `{"config": null}`.
That's because the config property is defined with a default value of
NULL, hence it was included. The other properties are not included
because they don't have a default property, and nothing is ever written
to their backing field. Hence, the JSON encoder excludes them.
Similarly, `(array) $doc` would yield the same `config` key in the
array.

Closes phpGH-11840.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants