Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource to opaque object migration #6

Open
29 of 41 tasks
Girgias opened this issue Jun 16, 2020 · 17 comments
Open
29 of 41 tasks

Resource to opaque object migration #6

Girgias opened this issue Jun 16, 2020 · 17 comments

Comments

@Girgias
Copy link
Member

Girgias commented Jun 16, 2020

A long term project is to move all extension resources to objects as they are all in all better.

This issue aims to list the remaining usages of resources within php-src

Related RFC: https://wiki.php.net/rfc/resource_to_object_conversion

PHP 8.0:

PHP 8.1:

Scheduled for PHP 8.4:

Scheduled for PHP 9.0:

Remaining:

  • stream (main/, ext/standard/) [libxml_set_streams_context() is also affected]
    • file/dir streams: le_stream / le_pstream
    • context: le_stream_context
    • filter: le_stream_filter
    • Bucket API: le_bucket / le_bucket_brigade

Persistent resources that are only used internally and not exposed to userland:

  • MySQLi: le_pmysqli
  • PDO: le_ppdo
  • PGSQL: le_plink
  • ODBC: le_pconn
  • DBA: le_pdb
@remicollet
Copy link

About ZIP, I don't know if it worth the work, as we already have a full OO API.
Perhaps we should rather deprecate the old functional and minimal API.

@kocsismate
Copy link
Member

@remicollet I was wondering about this a while ago. See my question at php/php-src#5601 (comment)

I even had a try at the conversion, but for me, it didn't go as easy as it seemed first, so I stopped. As far as I remember, my main problem was that the data structures used by the OO and procedural interface don't align really well.

Is it really the case? If so, then I'd also prefer deprecating the old API.

@Ayesh

This comment has been minimized.

@Girgias

This comment has been minimized.

@Ayesh

This comment has been minimized.

@cmb69

This comment has been minimized.

@nikic
Copy link
Member

nikic commented Jun 22, 2020

Because this is an anticipated failure mode of hosting this internal coordination tracker on GitHub, I'm going to err on the side of over-moderation and say: This is not the place to have this discussion. These issues have been discussed extensively on the PHP internals list in the past, and that would also be the place to revisit any decisions.

@sgolemon
Copy link

sgolemon commented Aug 5, 2020

BZ2 should be filed under ext/standard/streams
The only resources it interacts with are streams.

@sgolemon
Copy link

sgolemon commented Aug 6, 2020

Regarding streams (which are going to be biggest PITA by far), I've been thinking it might make transition smoother (and make calling semantics generally nicer for extension authors) to add a new ZPP type F which returns a php_strea*, so:

  php_stream *stream;

  if (zend_parse_parameters(ZEND_NUM_ARGS(), "F", &stream) == FAILURE) {
    RETURN_THROWS();
  }

The idea being that we can update the rest of the runtime where streams are taken as arguments to remove any remaining hint of resources first, then work on the internals without a massive chunk of the userspace API breaking underneath us. Obviously a handful of functions will still need special attention (fclose(), a few stream_*() functions), but they'll be the minority.

We could even update the stubs file with a not-yet-extant "File" class and initially have the arginfo generator regard this as ZEND_ARG_TYPED_INFO(IS_RESOURCE), but when we do the big switchover, just have the generator now use ZEND_ARG_OBJ_INFO(File). Again, this should let us focus on the hard parts without being bogged down by a thousand callsites.

Edit to add: I just remembered why I never moved on this. php_stream is a PHP thing, Zend shouldn't call back to PHP. Back to the drawing board.

@sgolemon
Copy link

sgolemon commented Aug 6, 2020

Okay, expanded idea:

typedef void* (zend_parse_param_callback*)(zval *arg);

// In zpp arg handler:
    case 'R': { /* Resolver */
        void **ptr = va_arg(*va, void **);
        zend_parse_param_callback *callback = va_arg(*va, zend_parse_param_callback*);

        *ptr = callback(arg);
        if (!*ptr) {
            ZEND_ASSERT(EG(exception));
            return "";
       }
       break;
    }

Then callsites look like:

{
    php_stream *stream;

    if (zend_parse_parameters(ZEND_NUM_ARGS(), "R", &stream, php_stream_from_zval) == FAILURE) {
        RETURN_THROWS();
    }
}

Edit to add: While php_stream_from_zval() doesn't currently throw exceptions, I'm sure we could use a version which does throw a proper TypeError.

@sgolemon
Copy link

sgolemon commented Aug 6, 2020

On the other hand.... I've just actually sat down to do grep -r php_stream_to_zval ext/ | wc -l and the answer is only 60. So I may be making a tempest in a teapot.

wmfgerrit pushed a commit to wikimedia/mediawiki-tools-codesniffer that referenced this issue Sep 17, 2020
PHP 8.0+ is migrating away from the `resource` type in favor of normal
classes, see <php/php-tasks#6>.

Code should generally check for return values being false rather than using
is_resource().

Bug: T260735
Change-Id: Iad84a60b302b22497f7ec1282e4e6223a32ebaeb
@jrfnl
Copy link

jrfnl commented Oct 29, 2020

For completeness of the list, the following extensions also saw resource to object changes in PHP 8.0:

The status of the Enchant extension is not 100% clear to me. The change from resource to object is mentioned in the changelog, but the associated PR discussion gives the impression that the commit got reverted: php/php-src#5533

@jrfnl
Copy link

jrfnl commented Oct 29, 2020

Another one for which I can't seem to determine the status is SNMP.
I can see some PHP 8.0 commits mentioning a resource to object change, but I can't find an entry in the changelog, nor an associated PR:

@Girgias
Copy link
Member Author

Girgias commented Oct 29, 2020

For completeness of the list, the following extensions also saw resource to object changes in PHP 8.0:

* GD, GdImage  - [php/php-src#4714](https://github.com/php/php-src/pull/4714)

* XML - [php/php-src#3526](https://github.com/php/php-src/pull/3526)

* XMLWriter - [php/php-src#4706](https://github.com/php/php-src/pull/4706)

The status of the Enchant extension is not 100% clear to me. The change from resource to object is mentioned in the changelog, but the associated PR discussion gives the impression that the commit got reverted: php/php-src#5533

Have a look at commit: php/php-src@cd3e04d for Enchant

@jrfnl
Copy link

jrfnl commented Oct 29, 2020

Thanks @Girgias !

@cmb69
Copy link

cmb69 commented Oct 29, 2020

Another one for which I can't seem to determine the status is SNMP.

Me neither, but it seems there is an OOP API as of PHP 5.4.0 (http://github.com/php/php-src/commit/5e82e334ddffcf577542a74a37f3388d14790686) which is not documented at all.

PS: oh, it is documented.

@jrfnl
Copy link

jrfnl commented Oct 30, 2020

@cmb69 Thanks for taking a look! Still not entirely clear what the change in PHP 8.0 is related to that. Let's hope someone figures it out and adds it to the changelog (if relevant enough) ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants