Migrate ext/soap resources to objects #14121
Conversation
|
There's two simple failures due to different object numberings, that should be easy to fix. |
Yes, these are fixed locally. I'll look into the memory leak related failure tomorrow. :)
I may try it, sure. |
ext/soap/soap.stub.php
Outdated
| /** @tentative-return-type */ | ||
| public function __setLocation(?string $location = null): ?string {} | ||
| } | ||
| } |
@Girgias Please use the "hide whitespace changes" feature to fix it ^^ But anyway, since I'm currently rewriting the history completely, I'll commit this separately. |
These cause test failures when we migrate resources to objects. But anyway, hardcoding the object IDs and the number of properties is hardly ever useful, so it's fine to get rid of them. Related to #14121
These cause test failures when we migrate resources to objects. But anyway, hardcoding the object IDs and the number of properties is hardly ever useful, so it's fine to get rid of them. Related to #14121
UPGRADING
Outdated
| @@ -135,6 +135,10 @@ PHP 8.4 UPGRADE NOTES | |||
| . Calling simplexml_import_dom() with a non-XML object now throws a TypeError | |||
| instead of a ValueError. | |||
|
|
|||
| - SOAP: | |||
| . SoapClient::$httpurl is now a Soap\Url object rather than a resource. | |||
| Value checks using is_resource() should be replaced with checks for `null`. | |||
There was a problem hiding this comment.
I first didn't understand what this last sentence meant, but I guess you mean that you have to take into account it can be a null value, so you can't pass it to is_resource? However, is_resource will always return false for Soap\Url, so I'm not sure what you mean.
There was a problem hiding this comment.
This is the upgrading note template we used in the past for all the other conversions, but I'm happy to change it in the name of clarity ^^ It is intended to mean that when checking whether the property "has a value", then previously one may have used is_resource($client->httpurl), but obviously this won't work anymore, so it should be changed to $client->httpurl !== null. Of course, it's mostly a theoretical problem IMO, since the property is private.
There was a problem hiding this comment.
Okay I get it now, thx for explaining.
I think this is unclear indeed, because "value checks" is quite vague. is_resource always checks a value, so writing "value checks using is_resource()" was very weird to me.
There was a problem hiding this comment.
I removed the "value" word, and added an example. I think that's the most clear we can do. I'll merge my changes, but if the note is still unclear, we can amend it separately.
ext/soap/soap.stub.php
Outdated
| * @strict-properties | ||
| * @not-serializable | ||
| */ | ||
| class Url |
There was a problem hiding this comment.
Should this be final? Intuitively I'd think so.
There was a problem hiding this comment.
Yes, I forgot about the final modifier, but it's a good idea!
UPGRADING
Outdated
| @@ -138,6 +138,8 @@ PHP 8.4 UPGRADE NOTES | |||
| - SOAP: | |||
| . SoapClient::$httpurl is now a Soap\Url object rather than a resource. | |||
| Value checks using is_resource() should be replaced with checks for `null`. | |||
| . SoapClient::$sdl is now a Soap\Sdl object rather than a resource. | |||
| Value checks using is_resource() should be replaced with checks for `null`. | |||
There was a problem hiding this comment.
Same remark as earlier about the last sentence.
ext/soap/soap.stub.php
Outdated
| * @strict-properties | ||
| * @not-serializable | ||
| */ | ||
| class Sdl |
ext/soap/php_soap.h
Outdated
| typedef struct { | ||
| php_url *url; | ||
| zend_object std; | ||
| } soap_url_object; |
There was a problem hiding this comment.
| typedef struct { | |
| php_url *url; | |
| zend_object std; | |
| } soap_url_object; | |
| typedef struct soap_url_object { | |
| php_url *url; | |
| zend_object std; | |
| } soap_url_object; |
Can you please add a name to the struct, instead of typedefing an anonymous struct? This makes the output of tools like "pahole" easier to understand. The same remark applies further below and possibly in other files as well.
Part of php/php-tasks#6. Migrates 2 out of 3 resource types in ext/soap to opaque objects.