Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jan 16, 2023

RFC: https://wiki.php.net/rfc/file-descriptor-function

The purpose of this function is to retrieve the underlying file descriptors for PHP streams when they exist.
It is currently possible to achieve this result by using FFI and stubbing the Zend engine (see: https://github.com/ppelisset/php-fileno).
This can be needed when trying to interact with a USB device, as this is the use case @ppelisset has.

Test should be based of #10173 when it gets merged.

* It is only used here so that the buffered data warning is not displayed.
*/
if (php_stream_can_cast(stream, PHP_STREAM_AS_FD | PHP_STREAM_CAST_INTERNAL) == FAILURE) {
zend_argument_type_error(1, "cannot represent as a file descriptor");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
zend_argument_type_error(1, "cannot represent as a file descriptor");
zend_argument_type_error(1, "cannot be represented as a file descriptor");

@azjezz
Copy link

azjezz commented Feb 1, 2023

it think we miss a test case for closed resources, i.e:

$fp = fopen('somefile.txt', 'wb+');
fclose($fp);

try {
  var_dump(file_descriptor($fp));
} catch ( ... ) {
  ...
}


php_stream_from_zval(stream, zsrc);

/* TODO Should support streams that can be cast with PHP_STREAM_AS_FD_FOR_SELECT ? */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general PHP_STREAM_AS_FD should be supported if PHP_STREAM_AS_FD_FOR_SELECT which should be the case for all core stream wrappers - I just checked and don't see a case where it would be different. So I wouldn't bother with checking PHP_STREAM_AS_FD_FOR_SELECT as well.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good in general. The test failure seems related as you probably know.

@Girgias Girgias requested a review from kocsismate as a code owner April 15, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants