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

implement php://fdraw stream wrapper #1281

Closed
wants to merge 3 commits into from

Conversation

6 participants
@tony2001
Copy link
Contributor

commented May 14, 2015

This wrapper differs from php://fd/ in one aspect: it doesn't call dup() on the descriptor, which means you fully control the original fd and can even close() it.
The patch also adds "fd" element to stream_get_meta_data() function, which contains fd number, if it's available.

Initially implemented by @YuriyNasretdinov.

tony2001 added some commits May 14, 2015

add php://fdraw/ stream wrapper
this patch adds ability to access the original (not dup()'ed) file
descriptor in a script, as opposed to php://fd/
@Kubo2

This comment has been minimized.

Copy link
Contributor

commented May 14, 2015

It would be nice to name it some more descriptive way, like php://fd/<n>/raw, because at the first sight it seems like "ef draw". That is my opinion.

@smalyshev

This comment has been minimized.

Copy link
Contributor

commented May 16, 2015

stream_get_meta_data() seems to still fail.

@tony2001

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2015

@smalyshev please elaborate.

@anton-povarov

This comment has been minimized.

Copy link

commented Jun 22, 2015

@smalyshev - could you please take another look at this?

fildes_ori = ZEND_STRTOL(start, &end, 10);
if (end == start || *end != '\0') {
php_stream_wrapper_log_error(wrapper, options,
"php://fd/ stream must be specified in the form php://fd/<orig fd>");
"php://fd%s/ stream must be specified in the form php://fd/<orig fd>", is_raw ? "raw" : "");

This comment has been minimized.

Copy link
@smalyshev

smalyshev Jun 29, 2015

Contributor

shouldn't the second php://fd also have %s?

if (fcntl(fd, F_GETFD) == -1) {
#else
#warning "No API to check file descriptor for this operating system, php://fdraw/ will not work"
if (0) {

This comment has been minimized.

Copy link
@smalyshev

smalyshev Jun 29, 2015

Contributor

I'd rather have if(1) here... I.e. if no way to check, then every descriptor is invalid. Otherwise it may be too dangerous?

@smalyshev

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2015

Looks ok to me except for small notes inline.

@Kubo2

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2015

@tony2001 What do you say to my first note here?

@anton-povarov

This comment has been minimized.

Copy link

commented Oct 27, 2015

i get this feeling - that this pull is never going to happen :(

@a-stepanenko

This comment has been minimized.

Copy link

commented Nov 6, 2015

What should be done to get this pull request merged? Is fd//raw instead of fdraw/ really neccessary?

@Kubo2

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2015

Can't you just propose php://fd//raw instead of php://fdraw? Or make an
RFC vote...
Anyway, I don't think that is mandatory, only that it's not its time yet.

On 6 November 2015 at 15:16, a-stepanenko notifications@github.com wrote:

What should be done to get this pull request merged? Is fd//raw instead of
fdraw/ really neccessary?


Reply to this email directly or view it on GitHub
#1281 (comment).

Cheers,
Kubis

@tony2001

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2015

Can't you just propose php://fd//raw instead of php://fdraw?

Why? What's the problem with 'fdraw' prefix?
You insist on inventing additional suffixes just because you alone saw 'fDRAW' instead of 'FDraw'. Honestly, this is looks weird to me.

Or make an RFC vote...

There are 3.5 people interested in this feature, I don't think there will be more if we create an RFC.

@Kubo2

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2015

On 6 November 2015 at 16:08, Antony Dovgal notifications@github.com wrote:

Can't you just propose php://fd//raw instead of php://fdraw?

Why? What's the problem with 'fdraw' prefix?

There's not. He was asking, I only repeated my opinion.

You insist on inventing additional suffixes just because you alone saw
'fDRAW' instead of 'FDraw'. Honestly, this is looks weird to me.

I do because readability is an important part of your code. Are you gonna
write a comment for every use of it instead?

Or make an RFC vote...
There are 3.5 people interested in this feature, I don't think there will
be more if we create an RFC.

Hm, here you're right, then after posting I only realised there's no RFC
for this PR.


Reply to this email directly or view it on GitHub
#1281 (comment).

Cheers,
Kubis

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 1, 2017

@tony2001 fix conflicts please ... I would have merged this into master today ...

@krakjoe

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

Having waited more than a month for feedback, and having received nothing, I'm closing this PR as it would appear abandoned.

Shame, I liked this ... please take this action as encouragement to open a clean PR against an appropriate branch.

@krakjoe krakjoe closed this Feb 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.