Convert resource to object in XML-RPC extension #5457
Conversation
ext/xmlrpc/xmlrpc-epi-php.c
Outdated
| @@ -744,42 +785,31 @@ PHP_FUNCTION(xmlrpc_server_create) | |||
| } | |||
|
|
|||
| if (USED_RET()) { | |||
There was a problem hiding this comment.
Do we really need this check? If not, can the other usages be removed (from array.c for example)?
There was a problem hiding this comment.
In this case, the USED_RET() check makes no sense. The ones in array.c are present because the functions both have a side-effect and a return value, and the check saves computing the return value if it isn't used.
There was a problem hiding this comment.
I got rid of all the USED_RET() checks
| * destroy_server_data(server); | ||
| */ | ||
| RETURN_BOOL(bSuccess == SUCCESS); | ||
| RETURN_TRUE; |
|
Thanks for working on this, but I think we should unbundle ext/xmlrpc for PHP 8, and if we do that it might be best to leave the extension as is. |
|
@cmb69 Can you suggest another extension instead that is worth the effort? |
|
Yes, certainly makes sense to start with a smaller extension first, but that might not be that simple, because anything that uses stream resources is better left alone for now, and xmlrpc doesn't seem the only extension which might better be unbundled (e.g. ext/ftp doesn't appear that useful anymore since it doesn't support SFTP, and it is barely maintained). The sys* extensions may be a reasonable start, but I barely know these since they're POSIX only. |
|
Unbundling after object conversion should not be a problem. I think we should proceed under the assumption that all extensions are going to stay, because most likely, that is exactly what is going to happen. If you're serious about unbundling something for PHP 8, please start an RFC asap. I think the only realistic candidates there are ext/xmlrpc and ext/imap though, because those depend on unmaintained libs. I doubt we're going to drop ext/ftp. |
|
Yeah, that would have been my next question if I see right that streams should be omitted? What makes them different? Or only the fact that they are much more wildly used? |
|
And yes, I also agree with Nikita in this question. Although this extension seems pretty useless, its conversion wasn't a really big effort and yet, there will be one less BC break if we would be able to remove support for resources completely, let's say in PHP 9. 🤞 |
|
Streams are special, because they are likely used by external extensions, and we may want to give the respective maintainers ample time to switch. |
kocsismate
force-pushed
the
xmlrpc-resource
branch
4 times, most recently
from
ff36b77 to
0514c09
Compare
kocsismate
force-pushed
the
xmlrpc-resource
branch
from
0514c09 to
8c41a2d
Compare
|
@nikic Can you please have a look at this before xml-rpc gets unbundled? :) I didn't noticed any complication, so I really hope there won't be too many issues with the implementation |
There are 3 memory leaks indicated inext/xmlrpc/tests/bug50285.phpt, but otherwise the tests already pass (locally). Will continue later :)