Add missing properties to xsl stub #12334
Conversation
ext/xsl/php_xsl.stub.php
Outdated
| @@ -71,6 +71,12 @@ | |||
|
|
|||
| class XSLTProcessor | |||
| { | |||
| /** @var bool */ | |||
| public $doXInclude; | |||
There was a problem hiding this comment.
I'd strongly prefer to have native types :) For PHP 8.1, I declared type for basically all untyped properties where it was possible so it would be very unfortunate to break the trend. :)
As far as I see, the correct type is?bool, but I'm not sure what we do with the xinclude field afterwards? Somehow I couldn't find any reference to it... 🤔
There was a problem hiding this comment.
BTW: wouldn't it be better to add a dedicated property handler instead of having a property and a field in the internal structure?
There was a problem hiding this comment.
Thanks for checking.
I'd strongly prefer to have native types :) For PHP 8.1, I declared type for basically all untyped properties where it was possible so it would be very unfortunate to break the trend. :)
Alright, just wasn't too sure regarding BC. I'll swap over to actual types and add a BC note to UPGRADING.
Also changed the code to use zend_is_true and use the R fetch instead of IS (as the property is now always defined).
As far as I see, the correct type is?bool
I believe just a bool would do. Because:
- For cloneDocument, the behaviour for false and null would be the same, so it makes sense to default the property to false.
- For doXInclude, the default behaviour set out by libxslt is false, so it makes sense we default the property to false. Again like the previous item: null and false would be the same so null has no reason to exist :)
but I'm not sure what we do with the xinclude field afterwards? Somehow I couldn't find any reference to it... 🤔
It's just passed to libxslt.
BTW: wouldn't it be better to add a dedicated property handler instead of having a property and a field in the internal structure?
The internal structure you're referring to only exists temporarily during the xslt evaluation. While possible to add a handler, we'd have to store and manage the value ourselves into a non-temporary object. So I think that would only complicate things.
There was a problem hiding this comment.
Alright, just wasn't too sure regarding BC. I'll swap over to actual types and add a BC note to UPGRADING.
IMO this is a small BC break which can easily be detected and fixed by end-users, so it should be fine to have even in a minor version. :)
And thanks for the rest of the answers!
This was added in 8d1427d, but never defined on the stub. It was more or less fine when dynamic properties were not deprecated, but now they throw a deprecation warning. To fix it, define on the stub. This should also help discoverability of the functionality.
This was introduced in 5c039bb, but never defined on the stub. It was more or less fine when dynamic properties were not deprecated, but now they throw a deprecation warning. To fix it, define on the stub. This should also help discoverability of the functionality.
Master-only, because this can technically break subclasses.
Might also be worth thinking about their types. They don't have one right now, and even though they act like booleans, internally the code considers them longs. It converts the zval to a long and if it's not 0 it will enable the behaviour, which is a bit unoptimal... Instead, using
zend_is_truewould be better but I'm not sure if we can just do that?