cc @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.

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

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

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.

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.

Thanks everyone for the feedback!

Applied the patch as 0966941.

@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/[email protected]

@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?

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"]=>

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

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

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

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.

UPGRADING note added with commit 49762c8.