Fix #79271: DOMDocumentType::$childNodes is NULL

[cmb69]

Dom level 2 core, DOM level 3 core and the DOM living standard agree
that childNodes always return a NodeList, and never null.

[cmb69]

BTW, what's an "interator"?

[cmb69]

cc @beberlei

[beberlei]

This is technically a BC break, but the likelihood of people working with DOMDocumentType directly tends towards 0, so I am fine with it. Wondering if this affects other node classes.

[cmb69]

Yes, that is a BC break, so maybe should target an higher branch? Anyway, these node types are affected as well.

[beberlei]

@theseer @ThomasWeinert can I loop you in for a comment?

[ThomasWeinert]

I think the change has little chance to break something. It would make a condition unnecessary (is the property a node list / not null) but in any case the user has to consider an empty node list.

[theseer]

I basically agree with @ThomasWeinert.

A Nodelist could always be empty and any implementation that doesn't check for that might (maybe should?) be considered buggy. It still is a BC break though, as one might have concluded from the property not being null that - logically - the list cannot be empty.

Question is what the real world impact of this change or break would be though, as I don't believe there to be many active uses of DOMDocumentType do begin with and it's property childNodes in particular.

But how many users does it have to affect before it's a BC break to avoid? ;-)
I'd still say it's a valid change to fix a bug for something that doesn't work according to specs and code, in case it didn't check for empty lists, should just get adjusted.

[cmb69]

Thanks everyone for the feedback!

Applied the patch as 0966941.

[beberlei]

@cmb69 Derick reported a change in xdebug tests failing on 7.3, is this change related? I don't see how on the first glance. https://xdebug.org/ci?r=2020-24-24-03-20-01@7.3dev-zts

[cmb69]

@beberlei, @derickr, this test failure is to be expected after this commit; the Text node now has an empty NodeList as childNodes instead of NULL before. Revert this commit from 7.3 (and maybe 7.4), or stick with the BC break?

[cmb69]

Alternatively, maybe add a node to UPGRADING?

Test fix:

 tests/base/bug00913.phpt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/base/bug00913.phpt b/tests/base/bug00913.phpt
index cd451134..3be71334 100644
--- a/tests/base/bug00913.phpt
+++ b/tests/base/bug00913.phpt
@@ -181,7 +181,7 @@ object(DOMText)#%d (%d) {
   ["parentNode"]=>
   string(22) "(object value omitted)"
   ["childNodes"]=>
-  NULL
+  string(22) "(object value omitted)"
   ["firstChild"]=>
   NULL
   ["lastChild"]=>

[derickr]

This isn't the only change in the test I think, there are also these:

056+   ["nextSibling"]=>
057+   NULL

[cmb69]

@derickr, nope, these are otherwise catched by the %as; it's just a glitch of the test runner's diff that these are reported.

[theseer]

In case it matters: I'd keep the - as stated before - minor BC break but adding a note might be a good idea never the less.

[cmb69]

UPGRADING note added with commit 49762c8.