Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

fix bug 65667 #478

Closed
wants to merge 2 commits into from

5 participants

Philip Hofstetter Michael Wallner Account for PHP Pull Requests Nikita Popov Lior Kaplan
Philip Hofstetter

This PR is a fix for bug 65667

the idea behind ftp_nb_get is for it to be followed by multiple calls
to ftp_nb_continue in order to download a file piece-by-piece.

As such, it's unwise to close the stream used to write the downloaded
data to when the file hasn't been completely downloaded within the first
call to ftp_nb_get.

This regression was added in a93a462
and this patch restores the behavior that was seen pre-patch.

Philip Hofstetter

(ext/session/tests/session_set_save_handler_class_005.phpt which failed the build also fails without my patch)

Michael Wallner

Yeah, the test suite is not 100% stable on travis yet.

Michael Wallner

So, the test suite doesn't fail now and I suspect it won't fail after merging your PR :) i.e. proof is missing.
Mind adding a test? :)

added some commits October 02, 2013
Philip Hofstetter fix bug 65667
the idea behind ftp_nb_get is for it to be followed by multiple calls
to ftp_nb_continue in order to download a file piece-by-piece.

As such, it's unwise to close the stream used to write the downloaded
data to when the file hasn't been completely downloaded within the first
call to ftp_nb_get.

This regression was added in a93a462
and this patch restores the behavior that was seen pre-patch.
1cc9ab8
Philip Hofstetter added testcase for bug 65667
fails to download the whole file because ftp_nb_get's implementation
closes the stream too early
1a06fd2
Philip Hofstetter

ok. I've added a testcase now.

I've rebased the commits such that the failing test case comes before the commit which fixes the bug.

Aside of that, let me switch into rant-mode for a second:

Here's what I've done for this bug:

  • After getting bitten by the issue (caused by a undocumented feature-addition to a point release) I searched the bug database in order to not report a duplicate
  • After confirming seeing the bug on my part, I went ahead and investigated the cause
  • I fixed the issue (respecting whitespace conventions - contrary to the commit that introduced the bug, btw).
  • I went through the history of the file in order to find the commit where the bug was introduced and pointed to that in the PHP bug
  • I commented on the offending commit, noting the bug as it was introduced.

After doing all that instead of a thank you and maybe even a merge of the PR, the thing I get is a complaint about a missing test. Yeah. I understand you'd like to see a testcase, but just consider what all went wrong to cause this bug:

  • The test-case attached to the commit that added the bug cheats around triggering the bug
  • The commit that caused the bug had inconsitent whitespace
  • The bug was introduced by changing lines that were never supposed to have been commited: the offending lines are completely unrelated to the commit and the commit message given.
  • The commit resulted in a feature-change in a point release which
  • was not documented in the changelog

As such, I would say that this commit should not have passed review, but yet it did.

On the other hand, my little fix which unbreaks the, now completely broken ftp_nb_get() gets complained about because of a missing test, despite the fact that there never was a test for ftp_nb_continue() and that it only re-adds code that was there before and likely removed by accident.

Come on.

Anyways. Rant over. As somebody using ftp_nb_get(), I'd naturally like to see this fixed :-)

Michael Wallner

Congrats, you've done a great job. Though, you could have reviewed at least 3 other bugs while writing this rant. The "Thank you" usually comes from a template when the bug is closed, but maybe we can get github to post a "Thank you" when requesting a pull?

Account for PHP Pull Requests
Collaborator

Comment on behalf of nikic at php.net:

Merged in 96cc419.

Account for PHP Pull Requests php-pulls closed this October 04, 2013
Nikita Popov

@pilif I'm sorry that the incorrect change in a93a462 got through code review. We prefer having a test for every change for exactly that reason: The test will prevent the regression from ever being introduced again. Even if the reviewer misses the change, the test will not ;)

Philip Hofstetter

@nikic no worries. Thank you so much for applying this and sorry for my rant - I guess I had a bad day today :(

Lior Kaplan

@pilif As long as you keep providing patches, you're welcome to rant (:

Philip Hofstetter

If only my C was better - then you'd get plenty of them. Until then I'll have to constrain myself to the low-hanging fruits :-)

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

Showing 2 unique commits by 1 author.

Oct 04, 2013
Philip Hofstetter fix bug 65667
the idea behind ftp_nb_get is for it to be followed by multiple calls
to ftp_nb_continue in order to download a file piece-by-piece.

As such, it's unwise to close the stream used to write the downloaded
data to when the file hasn't been completely downloaded within the first
call to ftp_nb_get.

This regression was added in a93a462
and this patch restores the behavior that was seen pre-patch.
1cc9ab8
Philip Hofstetter added testcase for bug 65667
fails to download the whole file because ftp_nb_get's implementation
closes the stream too early
1a06fd2
This page is out of date. Refresh to see the latest.
2  ext/ftp/php_ftp.c
@@ -968,7 +968,9 @@ PHP_FUNCTION(ftp_nb_get)
968 968
 		RETURN_LONG(PHP_FTP_FAILED);
969 969
 	}
970 970
 
  971
+	if (ret == PHP_FTP_FINISHED){
971 972
         php_stream_close(outstream);
  973
+	}
972 974
 
973 975
 	RETURN_LONG(ret);
974 976
 }
179  ext/ftp/tests/ftp_nb_continue.phpt
... ...
@@ -0,0 +1,179 @@
  1
+--TEST--
  2
+Testing whether ftp_nb_continue() fetches more data
  3
+--SKIPIF--
  4
+--FILE--
  5
+<?php
  6
+require 'server.inc';
  7
+
  8
+$file = "mediumfile.txt";
  9
+
  10
+$ftp = ftp_connect('127.0.0.1', $port);
  11
+ftp_login($ftp, 'user', 'pass');
  12
+if (!$ftp) die("Couldn't connect to the server");
  13
+
  14
+$local_file = dirname(__FILE__) . DIRECTORY_SEPARATOR . $file;
  15
+touch($local_file);
  16
+
  17
+$r = ftp_nb_get($ftp, $local_file, $file, FTP_BINARY);
  18
+while ($r == FTP_MOREDATA) {
  19
+    $r = ftp_nb_continue ($ftp);
  20
+}
  21
+ftp_close($ftp);
  22
+
  23
+echo file_get_contents($local_file);
  24
+?>
  25
+--CLEAN--
  26
+<?php
  27
+@unlink(dirname(__FILE__) . DIRECTORY_SEPARATOR . "mediumfile.txt");
  28
+?>
  29
+--EXPECT--
  30
+This is line 0 of the test data.
  31
+This is line 1 of the test data.
  32
+This is line 2 of the test data.
  33
+This is line 3 of the test data.
  34
+This is line 4 of the test data.
  35
+This is line 5 of the test data.
  36
+This is line 6 of the test data.
  37
+This is line 7 of the test data.
  38
+This is line 8 of the test data.
  39
+This is line 9 of the test data.
  40
+This is line 10 of the test data.
  41
+This is line 11 of the test data.
  42
+This is line 12 of the test data.
  43
+This is line 13 of the test data.
  44
+This is line 14 of the test data.
  45
+This is line 15 of the test data.
  46
+This is line 16 of the test data.
  47
+This is line 17 of the test data.
  48
+This is line 18 of the test data.
  49
+This is line 19 of the test data.
  50
+This is line 20 of the test data.
  51
+This is line 21 of the test data.
  52
+This is line 22 of the test data.
  53
+This is line 23 of the test data.
  54
+This is line 24 of the test data.
  55
+This is line 25 of the test data.
  56
+This is line 26 of the test data.
  57
+This is line 27 of the test data.
  58
+This is line 28 of the test data.
  59
+This is line 29 of the test data.
  60
+This is line 30 of the test data.
  61
+This is line 31 of the test data.
  62
+This is line 32 of the test data.
  63
+This is line 33 of the test data.
  64
+This is line 34 of the test data.
  65
+This is line 35 of the test data.
  66
+This is line 36 of the test data.
  67
+This is line 37 of the test data.
  68
+This is line 38 of the test data.
  69
+This is line 39 of the test data.
  70
+This is line 40 of the test data.
  71
+This is line 41 of the test data.
  72
+This is line 42 of the test data.
  73
+This is line 43 of the test data.
  74
+This is line 44 of the test data.
  75
+This is line 45 of the test data.
  76
+This is line 46 of the test data.
  77
+This is line 47 of the test data.
  78
+This is line 48 of the test data.
  79
+This is line 49 of the test data.
  80
+This is line 50 of the test data.
  81
+This is line 51 of the test data.
  82
+This is line 52 of the test data.
  83
+This is line 53 of the test data.
  84
+This is line 54 of the test data.
  85
+This is line 55 of the test data.
  86
+This is line 56 of the test data.
  87
+This is line 57 of the test data.
  88
+This is line 58 of the test data.
  89
+This is line 59 of the test data.
  90
+This is line 60 of the test data.
  91
+This is line 61 of the test data.
  92
+This is line 62 of the test data.
  93
+This is line 63 of the test data.
  94
+This is line 64 of the test data.
  95
+This is line 65 of the test data.
  96
+This is line 66 of the test data.
  97
+This is line 67 of the test data.
  98
+This is line 68 of the test data.
  99
+This is line 69 of the test data.
  100
+This is line 70 of the test data.
  101
+This is line 71 of the test data.
  102
+This is line 72 of the test data.
  103
+This is line 73 of the test data.
  104
+This is line 74 of the test data.
  105
+This is line 75 of the test data.
  106
+This is line 76 of the test data.
  107
+This is line 77 of the test data.
  108
+This is line 78 of the test data.
  109
+This is line 79 of the test data.
  110
+This is line 80 of the test data.
  111
+This is line 81 of the test data.
  112
+This is line 82 of the test data.
  113
+This is line 83 of the test data.
  114
+This is line 84 of the test data.
  115
+This is line 85 of the test data.
  116
+This is line 86 of the test data.
  117
+This is line 87 of the test data.
  118
+This is line 88 of the test data.
  119
+This is line 89 of the test data.
  120
+This is line 90 of the test data.
  121
+This is line 91 of the test data.
  122
+This is line 92 of the test data.
  123
+This is line 93 of the test data.
  124
+This is line 94 of the test data.
  125
+This is line 95 of the test data.
  126
+This is line 96 of the test data.
  127
+This is line 97 of the test data.
  128
+This is line 98 of the test data.
  129
+This is line 99 of the test data.
  130
+This is line 100 of the test data.
  131
+This is line 101 of the test data.
  132
+This is line 102 of the test data.
  133
+This is line 103 of the test data.
  134
+This is line 104 of the test data.
  135
+This is line 105 of the test data.
  136
+This is line 106 of the test data.
  137
+This is line 107 of the test data.
  138
+This is line 108 of the test data.
  139
+This is line 109 of the test data.
  140
+This is line 110 of the test data.
  141
+This is line 111 of the test data.
  142
+This is line 112 of the test data.
  143
+This is line 113 of the test data.
  144
+This is line 114 of the test data.
  145
+This is line 115 of the test data.
  146
+This is line 116 of the test data.
  147
+This is line 117 of the test data.
  148
+This is line 118 of the test data.
  149
+This is line 119 of the test data.
  150
+This is line 120 of the test data.
  151
+This is line 121 of the test data.
  152
+This is line 122 of the test data.
  153
+This is line 123 of the test data.
  154
+This is line 124 of the test data.
  155
+This is line 125 of the test data.
  156
+This is line 126 of the test data.
  157
+This is line 127 of the test data.
  158
+This is line 128 of the test data.
  159
+This is line 129 of the test data.
  160
+This is line 130 of the test data.
  161
+This is line 131 of the test data.
  162
+This is line 132 of the test data.
  163
+This is line 133 of the test data.
  164
+This is line 134 of the test data.
  165
+This is line 135 of the test data.
  166
+This is line 136 of the test data.
  167
+This is line 137 of the test data.
  168
+This is line 138 of the test data.
  169
+This is line 139 of the test data.
  170
+This is line 140 of the test data.
  171
+This is line 141 of the test data.
  172
+This is line 142 of the test data.
  173
+This is line 143 of the test data.
  174
+This is line 144 of the test data.
  175
+This is line 145 of the test data.
  176
+This is line 146 of the test data.
  177
+This is line 147 of the test data.
  178
+This is line 148 of the test data.
  179
+This is line 149 of the test data.
7  ext/ftp/tests/server.inc
@@ -368,6 +368,13 @@ if ($pid) {
368 368
                                         }
369 369
 					fputs($s, "226 Closing data Connection.\r\n");
370 370
                                         break;
  371
+                case "mediumfile":
  372
+					fputs($s, "150 File status okay; about to open data connection.\r\n");
  373
+					for($i = 0; $i < 150; $i++){
  374
+						fputs($fs, "This is line $i of the test data.\n");
  375
+					}
  376
+					fputs($s, "226 Closing data Connection.\r\n");
  377
+
371 378
 				default:
372 379
 					fputs($s, "550 {$matches[1]}: No such file or directory \r\n");
373 380
 					break;
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.