This patch corrects and completes all inline phpdoc documentation on PhpStreamWrapperInterface

CommentFileSizeAuthor
#18 drupal-2902424-18.patch14.42 KBVladimirAus
#18 interdiff-14-18.txt9.98 KBVladimirAus
#14 interdiff-4-14.txt2.62 KBVladimirAus
#14 drupal-2902424-14.patch12.37 KBVladimirAus
#4 2902424.2.patch12.37 KBphayes
#2 2902424.patch1.18 KBphayes
PhpStreamWrapperInterface.dir_readdir.patch522 bytesphayes
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phayes created an issue. See original summary.

phayes’s picture

phayes CreditAttribution: phayes commented
FileSize
2902424.patch1.18 KB

It appears that this is also a problem for:

- stream_read (http://php.net/manual/en/streamwrapper.stream-read.php)
- stream_stat (http://php.net/manual/en/streamwrapper.stream-stat.php / http://php.net/manual/en/function.stat.php)
- url_stat (http://php.net/manual/en/streamwrapper.url-stat.php / http://php.net/manual/en/function.stat.php)

I've attached another patch with these additional updates included.

phayes’s picture

phayes CreditAttribution: phayes commented
Status: Active » Needs review
phayes’s picture

phayes CreditAttribution: phayes commented
Title: return type on PhpStreamWrapperInterface::dir_readdir is wrong » Correcting and completing PhpStreamWrapperInterface inline documentation
Issue summary: View changes
FileSize
2902424.2.patch12.37 KB
phayes’s picture

phayes CreditAttribution: phayes commented
Title: Correcting and completing PhpStreamWrapperInterface inline documentation » Completing PhpStreamWrapperInterface inline documentation
phayes’s picture

phayes CreditAttribution: phayes commented
Component: documentation » file system

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Version: 8.9.x-dev » 9.1.x-dev
Issue tags: +Bug Smash Initiative

Patch still applies to 9.1.x. Kicking off tests for 9.1.x.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Needs review » Needs work

Thanks @phayes for working on this.

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * This method is called in response to closedir().
+   * Any resources which were locked, or allocated, during opening and use of

    Wrapped too early. 

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   *   Should return string representing the next filename,
+   *   or FALSE if there is no next file.

    Wrapped too early. 

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * This method is called in response to rewinddir().
+   * Should reset the output generated by streamWrapper::dir_readdir().
+   * The next call to streamWrapper::dir_readdir() should return the first
+   * entry in the location returned by streamWrapper::dir_opendir().

    Wrapped too early. 

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * This method is called in response to rename().
+   * Should attempt to rename $path_from to $path_to.

    Wrapped too early. 

  5. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * ¶

    Redundant trail whitespace. 

  6. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -65,21 +150,66 @@ public function stream_cast($cast_as);
+   *   Should return TRUE if the read/write position is at the end of the
+   *   stream and if no more data is available to be read, or FALSE otherwise.

    Wrapped too early.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Needs work » Needs review

Setting back to NR if someone wants to do an extra in-depth review.

VladimirAus’s picture

VladimirAus
Primary language English
Location Brisbane 🇦🇺
CreditAttribution: VladimirAus at Tomato Elephant Studio commented
FileSize
drupal-2902424-14.patch12.37 KB
interdiff-4-14.txt2.62 KB
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented

Thanks for the update. Reviewing the interdiff and comparing it against the items in #12, I don't see 12.6 addressed though I can't tell if "stream" would fit if it popped up a line just using dreditor.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented

Sorry I didn't check this properly before.

1) Nitpick: Just checked in an editor and see that "stream" can pop up a line for #12.6.

2) I reviewed all the text changes because it wasn't clear from the reviews above if the wording was reviewed or if it was just minor formatting reviewed. I went through each of the doc blocks and compared them against the php documentation that is being referenced:

  1. http://php.net/manual/en/streamwrapper.dir-closedir.php
  2. http://php.net/manual/en/streamwrapper.dir-opendir.php
  3. http://php.net/manual/en/streamwrapper.dir-readdir.php
  4. http://php.net/manual/en/streamwrapper.dir-rewinddir.php
  5. http://php.net/manual/en/streamwrapper.mkdir.php
  6. http://php.net/manual/en/streamwrapper.rename.php
  7. http://php.net/manual/en/streamwrapper.rmdir.php
  8. http://php.net/manual/en/streamwrapper.stream-close.ph
  9. http://php.net/manual/en/streamwrapper.stream-eof.php
  10. http://php.net/manual/en/streamwrapper.stream-flush.php
  11. http://php.net/manual/en/streamwrapper.stream-lock.php
  12. http://php.net/manual/en/streamwrapper.stream-open.php
  13. http://php.net/manual/en/streamwrapper.stream-read.php
  14. http://php.net/manual/en/streamwrapper.stream-stat.php
  15. http://php.net/manual/en/streamwrapper.stream-tell.php
  16. http://php.net/manual/en/streamwrapper.stream-truncate.php
  17. http://php.net/manual/en/streamwrapper.stream-write.php
  18. http://php.net/manual/en/streamwrapper.unlink.php
  19. http://php.net/manual/en/streamwrapper.url-stat.php

Most of the differences in the text from the patch compared to the php.net text were very minor re-formatting things that I won't note. But, here are some items that could be changed if desired:

  1. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * @return string|false
+   *   Should return string representing the next filename, or FALSE if there
+   *   is no next file.

    The php.net docs includes this note:

    The return value will be casted to string.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * @see rewinddir()

    The php.net "See Also" also includes:

    streamWrapper::dir_readdir()

  3. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * This method is called in response to mkdir().

    The php.net docs includes these notes:

    In order for the appropriate error message to be returned this method should not be defined if the wrapper does not support creating directories.

    The streamWrapper::$context property is updated if a valid context is passed to the caller function.

  4. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * @see mkdir()

    The php.net "See Also" also includes:

    streamwrapper::rmdir()

  5. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * This method is called in response to rename(). Should attempt to rename
+   * $path_from to $path_to.

    The php.net docs include these notes:

    In order for the appropriate error message to be returned this method should not be defined if the wrapper does not support renaming files.

    The streamWrapper::$context property is updated if a valid context is passed to the caller function.

  6. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * This method is called in response to rmdir().

    The php.net docs include these notes:

    In order for the appropriate error message to be returned this method should not be defined if the wrapper does not support removing directories.

    The streamWrapper::$context property is updated if a valid context is passed to the caller function.

  7. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,122 @@
+   * @see rmdir()

    The php.net "See Also" also includes:

    streamwrapper::mkdir()
streamwrapper::unlink()

  8. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -65,21 +150,66 @@ public function stream_cast($cast_as);
+   * @see fclose()

    The php.net "See Also" also includes:

    streamWrapper::dir_closedir()

  9. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -65,21 +150,66 @@ public function stream_cast($cast_as);
+   * This method is called in response to feof().

    The php.net docs has this warning:

    When reading the whole file (for example, with file_get_contents()), PHP will call streamWrapper::stream_read() followed by streamWrapper::stream_eof() in a loop but as long as streamWrapper::stream_read() returns a non-empty string, the return value of streamWrapper::stream_eof() is ignored.

  10. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -65,21 +150,66 @@ public function stream_cast($cast_as);
+   *   Should return TRUE if the cached data was successfully stored (or if
+   *   there was no data to store), or FALSE if the data could not be stored.
...
+   * @see http://php.net/manual/en/streamwrapper.stream-flush.php

    The php.net docs includes this note:

    If not implemented, FALSE is assumed as the return value.

  11. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -65,21 +150,66 @@ public function stream_cast($cast_as);
+   *   - LOCK_NBL: If you don't want flock() to block while locking. This

    The php.net docs has this as LOCK_NB. 

  12. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -65,21 +150,66 @@ public function stream_cast($cast_as);
+   * @see flock()

    The php.net "See Also" also includes:

    stream_set_blocking()

  13. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -116,12 +246,68 @@ public function stream_lock($operation);
+   * This method is called immediately after the wrapper is initialized (f.e.

@@ -183,12 +369,30 @@ public function stream_seek($offset, $whence = SEEK_SET);
+   * @see fseek()

    The php.net "See Also" has:

    streamWrapper::stream_tell()

    rather than:

    fseek()

  14. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -116,12 +246,68 @@ public function stream_lock($operation);
+   * This method is called immediately after the wrapper is initialized (f.e.

    Nitpick: "f.e." => "e.g.,". 

  15. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -116,12 +246,68 @@ public function stream_lock($operation);
+   * by fopen() and file_get_contents()). Note The `streamWrapper::$context`

    Nitpick: "Note The" => "Note the". 

  16. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -116,12 +246,68 @@ public function stream_lock($operation);
+   *   The mode used to open the file, as detailed for fopen(). Note: Remember

    Nitpick: "Note: " used here yet "Note " used elsewhere. 

  17. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -116,12 +246,68 @@ public function stream_lock($operation);
+   * were successfully read). Note that `streamWrapper::stream_eof()` is called
+   * directly after calling streamWrapper::stream_read() to check if EOF has
+   * been reached.

    The php.net docs has this note:

    streamWrapper::stream_eof() is called directly after calling streamWrapper::stream_read() to check if EOF has been reached. If not implemented, EOF is assumed.

    which is almost the same as the patch but the patch is missing the last sentence:

    If not implemented, EOF is assumed.

  18. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -116,12 +246,68 @@ public function stream_lock($operation);
+   * WARNING: When reading the whole file (eg. with file_get_contents()), PHP

    Nitpick: "eg." => "e.g.,". 

  19. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -202,21 +406,75 @@ public function stream_tell();
+   * This method is called in response to unlink().

    The php.net docs include these notes:

    In order for the appropriate error message to be returned this method should not be defined if the wrapper does not support removing files.

    The streamWrapper::$context property is updated if a valid context is passed to the caller function.

  20. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -202,21 +406,75 @@ public function stream_tell();
+   * @see unlink()

    The php.net "See Also" also includes:

    streamWrapper::rmdir()

  21. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -202,21 +406,75 @@ public function stream_tell();
+   * This method is called in response to all stat() related functions.

    The php.net docs include this note:

    The streamWrapper::$context property is updated if a valid context is passed to the caller function.

  22. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -202,21 +406,75 @@ public function stream_tell();
+   * @see stat()

    The php.net "See Also" also includes:

    streamwrapper::stream_stat()
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Needs work

Whoops. Back to needs work.

IMO, the only items from #16 that definitely should be addressed are:

  • 1
  • 2.9 (since it's a warning and a warning was added for a different doc block)
  • 2.11
  • 2.13 (maybe)
  • 2.14
  • 2.15
  • 2.18
VladimirAus’s picture

VladimirAus
Primary language English
Location Brisbane 🇦🇺
CreditAttribution: VladimirAus at Tomato Elephant Studio commented
Status: Needs work » Needs review
FileSize
interdiff-14-18.txt9.98 KB
drupal-2902424-18.patch14.42 KB

Thanks @Kristen Pol for thorough review:

Not updated

  • 1. stream sits at 81 characters.

Updated

  • 2.1-2.22 were updated.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Closed (duplicate)
Related issues: +#2228087: PhpStreamWrapperInterface lacks docblocks

I was starting to review this when I thought I had seen this before. Yes, this is a duplicate of an older issue, which is a child of the issue that added the PhpStremWrapperInterface. Because it is the older issue and has that history, I think this one should be closed as a duplicate and the work and credit moved over there including referencing the last review by Kirsten Pol over there to make sure that is addressed.