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

add freopen() with tests #950

Closed
wants to merge 4 commits into from

Conversation

9 participants
@tony2001
Copy link
Contributor

commented Dec 10, 2014

it works with plain_wrapper only, though
patch by Evgeniy Makhrov

particularly useful to redirect all the output to a file:

$str = "data\n";
$file = "stdout.log";
freopen($file, "a", STDOUT);
echo $str; //you won't see this line

@laruence

This comment has been minimized.

Copy link
Member

commented Dec 11, 2014

hmm, wo don't have dup.. so, how could I reopen repopened STDOUT to origin STDOUT?

@tony2001

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2014

Sorry, didn't get your question.

@laruence

This comment has been minimized.

Copy link
Member

commented Dec 11, 2014

I mean, hmm, how can I restore the freopen?

@tony2001

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2014

Well, there's a way. This works for me on Linux:
$str = "data\n";
$file = "stdout.log";
$fp = STDOUT;
freopen($file, "a", STDOUT);
echo $str;

freopen("/dev/tty", "a", $fp);
echo "Hello again\n";

Not sure if it's going to work on any other OS, though.
According to this http://c-faq.com/stdio/undofreopen.html you shouldn't be using freopen() if you need to revert it.

@laruence

This comment has been minimized.

Copy link
Member

commented Dec 11, 2014

hmm, I see your point.. anyway, I ping you on IRC about this.. "laruence> so, I was worring , if we provide freopen, then maybe we also needs some way to allow people to restore it. does it make sense to you?
"

@jpauli jpauli added the PHP7 label Dec 12, 2014

@Kubo2

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2014

@tony2001 If someone wants to redirect output, isn't PHP's OB enough?

<?php

ob_start(); {
   // some output
   echo("You won't see this");
} $out = ob_get_clean();

// redirect output to a file
file_put_contents(__DIR__ . '/stdout.log', $out);

// other output
echo("Hello again!");
@a-stepanenko

This comment has been minimized.

Copy link

commented Dec 14, 2014

OB is buffered so nothing would be written until we explicitly call flush
file_put_contents makes sense, but what if we have huge complicated script using echo, fwrite(STDIN), fwrite(STDERR) and we want to redirect all its output somewhere without painful refactoring?

@Kubo2

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2014

@a-stepanenko Aha, yes, here it really makes some sense using freopen().

@tony2001 tony2001 force-pushed the tony2001:freopen branch from a16e08c to 23beb73 Jan 22, 2015

@Kubo2

This comment has been minimized.

Copy link
Contributor

commented May 28, 2015

It seems that Travis CI build failure isn't related to this patch, but the freopen() tests didn't execute at all. I could probably test this manually today.

tony2001 added some commits Dec 10, 2014

add freopen() with tests
it works with plain_wrapper only, though
patch by Evgeniy Makhrov
zval *arg1;
php_stream *stream;

if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "psr", &filename, &filename_len, &mode, &mode_len, &arg1) == FAILURE) {

This comment has been minimized.

Copy link
@weltling

weltling May 29, 2015

Contributor

i was hoping not to see these TSRM thingies anymore :)

This comment has been minimized.

Copy link
@tony2001

tony2001 May 29, 2015

Author Contributor

Oh, somehow I've missed the moment those were removed.
I've nuked them all now.

@tony2001 tony2001 force-pushed the tony2001:freopen branch from 23beb73 to fc847f6 May 29, 2015

@@ -54,6 +54,9 @@ PHPAPI php_stream *_php_stream_fopen_temporary_file(const char *dir, const char
PHPAPI FILE * _php_stream_open_wrapper_as_file(char * path, char * mode, int options, zend_string **opened_path STREAMS_DC);
#define php_stream_open_wrapper_as_file(path, mode, options, opened_path) _php_stream_open_wrapper_as_file((path), (mode), (options), (opened_path) STREAMS_CC)

PHPAPI int _php_stream_freopen(char *filename, char *mode, php_stream *stream STREAMS_DC TSRMLS_DC);
#define php_stream_freopen(filename, mode, stream) _php_stream_freopen((filename), (mode), (stream) STREAMS_CC TSRMLS_CC)

This comment has been minimized.

Copy link
@weltling

weltling May 29, 2015

Contributor

Yeah, the above lines have two more TSRMLS ocurrences, but that's it probably.

@weltling

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

Tested on windows, there's one test fail

TEST 3/3 [ext\standard\tests\file\freopen_basic3.phpt]
========DIFF========
006+ string(0) ""
006- string(4) "test"

But besides that - I would suggest the same as @laruence . I've found a bit unusual loosing the original fd. IMHO the API should rather return the new fd. Except this fact, the idea is perfectly useful.

Thanks.

@tony2001

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2015

But that's kind of the point, isn't it? We get not the original fd, but the a duplicate which we can control separately.

@weltling

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

I get what you mean, clear. Just my arguments are - if we can do the roundup, fe with restoring STDOUT, then why don't exhaust that. IMHO this would enourmously improve the patch.

The page you've linked might be good correct for the C runtime, but the patch uses dup. That gives a possibility to save the original fd and then to dup it back. I'm talking particularly about like this in quick pseudo C

int save;
int file;

puts("stdout here");

save = dup(1);

file = open("my_file", O_WRONLY);
dup2(file, 1);

puts("file here");

dup2(save, 1);

pust("stdout again");

This would make the user absolutely flexible. And it would be portable.

Thanks.

@YuriyNasretdinov

This comment has been minimized.

Copy link

commented Aug 18, 2015

Actually there is a very simple (although maybe not well-documented) way of dup'ing the file descriptor. If you call $save = fopen("php://fd/1"); , you will get a copy of stdout because of implementation details. See #1281 for more information.

So the way to access the original pointer is pretty simple:

<?php
echo "Stdout here\n";
$save = fopen("php://fd/1");
freopen("stdout.log", "a", STDOUT);
echo "File here\n";
fwrite($save, "Stdout still here\n");
@weltling

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2015

@YuriyNasretdinov but it doesn't restore the regular stdout, all the echo will still go into the file. Please read the earlier post by @tony2001 how to do it, though non portable way.

Thanks.

@YuriyNasretdinov

This comment has been minimized.

Copy link

commented Aug 18, 2015

You mean /dev/tty? It will only restore original stdout if it wasn't already redirected into a pipe or file. It seems that indeed this patch must also introduce wrapper for dup2() syscall, it is not that complicated to implement, even for Windows

@weltling

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2015

@YuriyNasretdinov yeah, that's the exact point. It should be possible to restore the regular stdout at some point (not going into possible implementation details).

Thanks.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 1, 2017

This is tagged with 7.1, can I know the current status of the patch please ?

/cc @weltling @tony2001

@weltling

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2017

@krakjoe this patch was suggested for 7.0 as well. Just as mentioned above, it was asked to provide a way to restore the original descriptor and there was a test failure. Unfortunatelly the discussion stopped at that point :(

Thanks.

@krakjoe krakjoe added Waiting on Author and removed PHP7.1 labels Jan 3, 2017

@krakjoe

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

Having waited a month for feedback, and since the implementation raises unanswered questions, I'm closing this PR.

Please take this action as encouragement to open a clean PR against a supported branch, that addresses the concerns raised here.

@krakjoe krakjoe closed this Feb 3, 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.