Fixes GH-21768: ext/reflection: assertion failure on internal virtual p…

[devnexen]

…roperties.

ReflectionProperty::isReadable() and isWritable() asserted prop->hooks being set whenever ZEND_ACC_VIRTUAL is set. Internal classes such as DOMDocument declare virtual properties backed by the object read/write handlers without hooks, tripping the assertion.

Assertion introduced in e4f727d.

[ndossche]

So this solves the assertion failure. But I wonder what this new reflection method actually tests...

I.e. the following code will throw:

<?php
$dom = new DOMDocument;
$dom->doctype = null; // Error!

but it claims this property is writable:

<?php
$rc = new ReflectionClass($dom);
var_dump($rc->getProperty('doctype')->isWritable(null)); // bool(true)

[devnexen]

Fair point, returning true for read-only props like doctype is misleading. I'm not sure what the right call is here.

Would it make more sense to fix this on the DOM side, giving these @virtual properties proper get-only hooks so reflection can answer correctly, rather than working
around it in reflection itself? Hooks were designed for this shape, and it'd also benefit anything else that introspects them. It's a bigger patch, but probably the
more correct direction.

As a narrow alternative for the crash-fix, we could throw ReflectionException whenever the property is ZEND_ACC_VIRTUAL without hooks, something like "Cannot
determine writability of virtual property DOMDocument::$doctype without property hooks." Keeps the method honest (correct bool or "don't know") without pretending to
know.

Happy to go either way, or hold off on this PR if you'd rather sort it at the DOM level.

[ndossche]

The properties of dom that aren't writable are marked as readonly. This isn't really right as some dom manipulations can change those properties indirectly anyway. This is a leftover from back in the days.
It would be more correct to redeclare them as private(set) instead of readonly, in conjunction with this patch. This might break property redeclarations though so that's definitely master-only material.

[iluuu1994]

I think the prop->hooks should be moved to the if (prop->flags & ZEND_ACC_VIRTUAL) { check, and then adjusting the prop->flags & ZEND_ACC_READONLY to return false for @readonly internal properties. I think it's safe to assume they will always be initialized internally. That should work for the existing cases like in ext-date.

[devnexen]

Thanks, that's a cleaner shape. Two questions before I go that way.

ndossche suggested on the same PR that DOM's formerly @readonly @virtual properties should be redeclared as public private(set) instead, and I prototyped that on a
companion branch (https://github.com/php/php-src/pull/XXXX if I open it, otherwise local for now). With private(set), isWritable(null) already returns false for
DOMDocument::$doctype without touching reflection, because the existing visibility check at ZEND_ACC_PPP_SET_MASK fires first.

Your approach and hers overlap in intent, so I'd rather pick one direction than merge both. Do you have a preference?

The second question is whether the @readonly doc tag should start emitting ZEND_ACC_READONLY for internal virtual properties in gen_stub.php, since currently it's
documentation only. Without that, your short-circuit wouldn't fire on DOM because the flag isn't set. If we go your way, I'd add that to gen_stub.php alongside the
reflection change, but it would also apply the flag to a few non-virtual HTMLCollection $children cases, which needs a look to confirm the runtime readonly check
doesn't surprise anything.

And yes, I'll add Fixes GH-21768 to the PR body, sorry about the missing link.

[iluuu1994]

There are legitimate uses of virtual+readonly, even if dom is not one. So I think both should be fixed.

[iluuu1994]

I don't know the historical reason for @readonly and whether they can just be converted to readonly. But if possible that sounds reasonable. (Edit: I guess a bit weird for virtual properties.)

[iluuu1994]

@devnexen Btw, please also add "Fixes GH-xyz" to the description of your PRs. For some reason, GH does not properly link them when this text only appears in the title. See GH-21768, where this PR isn't listed under "Development".