Issue #3301239 by @immaculate.x : Check if the file is correctly opened before...
Issue #3301239 by @immaculate.x : Check if the file is correctly opened before trying to write the translation file contents to it.
Closes #3301239
Merge request reports
Activity
245 245 private function readLine() { 246 246 // Read a line and set the stream finished indicator if it was not 247 247 // possible anymore. 248 $line = fgets($this->fd); 248 $line = $this->fd ? fgets($this->fd) : FALSE; -
When the file descriptor is not set we should make sure an error is logged in
$this->errors[]and that we returnFALSE.It would probably more readable if we check that
$this->fdis set in a separate code block, something like this:if (empty($this->fd)) { $this->errors[] = new FormattableMarkup( ... ); return FALSE; } $line = fgets($this->fd); changed this line in version 14 of the diff
-
Looking through the rest of the class, I think
::readLine()is not the right place to fix it. If the file descriptor is not set we should not even try to read lines.Probably the best place to fix this is in
::open()which is responsible for creating the file descriptor. At the moment it just creates the descriptor without checking thatfopen()returns a valid resource:public function open() { if (!empty($this->uri)) { $this->fd = fopen($this->uri, 'rb'); $this->readHeader(); } else { throw new \Exception('Cannot open stream without URI set.'); } }If
fopen()returnsFALSEwe can throw an exception that the URI is invalid. added 1 commit
14 * @see Drupal\Component\Gettext\PoHeader. 15 * 16 * @group Gettext 17 */ 18 class PoStreamreaderTest extends TestCase { 19 20 /** 21 * This test validates that calling readItem with a NULL fd 22 * returns NULL. See issue #3301239 23 * 24 */ 25 public function testOpeningFileError() { 26 $reader = new PoStreamReader(); 27 $reader->setURI('fake'); 28 $this->assertNull($reader->readItem()); 29 } - Comment on lines +25 to +29
This method is not allowed to return
NULL, according toDrupal\Component\Gettext\PoReaderInterface::readItem(). The return value should have the typePoItem.Probably a good solution would be to not call
$reader->readItem()but instead call$reader->open(). This method should always be called prior to calling::readItem()anyway, and it allows exceptions to be thrown. And calling::open()caused::readItem()to be called as part of it.So a valid test could be to check that
::open()causes a helpful exception to be thrown if the URI is invalid.
added 1 commit
- 7db46e26 - We are doing our own error handling, suppress PHP warnings.
added 43 commits
-
7db46e26...101e0606 - 42 commits from branch
project:11.x - eaed2c58 - Merge branch drupal:11.x into 3301239-improve-error-message-and-exception-handling
-
7db46e26...101e0606 - 42 commits from branch
added 1 commit
23 * The PoStreamReader instance. 24 */ 25 private function createPoStreamReader(): PoStreamReader { 26 $reader = new PoStreamReader(); 27 $reader->setURI('fake'); 28 return $reader; 29 } 30 31 /** 32 * Calling open should throw an exception if URI is invalid. 33 * 34 * See issue #3301239. 35 */ 36 public function testOpenMethodThrowsExceptionOnInvalidURI(): void { 37 $reader = $this->createPoStreamReader(); 38 $this->expectException(Exception::class); changed this line in version 9 of the diff
34 * See issue #3301239. 35 */ 36 public function testOpenMethodThrowsExceptionOnInvalidURI(): void { 37 $reader = $this->createPoStreamReader(); 38 $this->expectException(Exception::class); 39 $reader->open(); 40 } 41 42 /** 43 * Validates that calling readItem with a NULL file descriptor returns NULL. 44 * 45 * See issue #3301239. 46 */ 47 public function testOpeningFileError(): void { 48 $reader = $this->createPoStreamReader(); 49 $this->assertNull($reader->readItem()); - Comment on lines +47 to +49
I think we should remove this test. It was a good way to start the work on test coverage for this issue, but if we keep this in we would be requiring that
$reader->readItem()SHOULD returnNULLwhen it is called in unsupported circumstances (by omitting the required call to$reader->open()first). And also according to the documentation$reader->readItem()should never returnNULL.This returning NULL is just a side effect of using the class incorrectly, we shouldn't formalize this behavior in a test.
I am happy with the first test
testOpenMethodThrowsExceptionOnInvalidURI(). I think it fully covers the requirements of the issue. changed this line in version 14 of the diff
added 765 commits
-
c6721a58...6439ca10 - 764 commits from branch
project:11.x - 55afa903 - Merge branch drupal:11.x into 3301239-improve-error-message-and-exception-handling
-
c6721a58...6439ca10 - 764 commits from branch
added 1 commit
added 1 commit
added 1 commit