PhpStreamWrapperInterface lacks docblocks

Let's extend this issue to correct/improve/unify/deduplicate documentation on classes that implement this interface, see #2213241-35: Fully conform to PHP5.4 streamwrapper class.

The parent is in. Can we get this in as well? I have not much time this month, but if no one else steps in, I will try to come up with a patch.

Closed #2902424: Completing PhpStreamWrapperInterface inline documentation as a duplicate. Uploading the latest patch from there to here, adding credit.
Then review that patch based on the comment from Kirsten Pol in 2902424#16.

The patch in #17 does address the issues raised in the duplicate in comment #16. That means that this is ready for review.

I do have a question. Why are we duplicating documentation that is the Php docs?

@quietone asked:

Why are we duplicating documentation that is the Php docs?

That's an excellent question :) I'm not sure. But, since that's what was being done, I reviewed based on that. Maybe it shouldn't have been done in the first place? Not sure what the protocol is for such things or who we should ask.

phpDoc is a API documenting standard and does not refer to a specific set of documentation, such as PHP core API documentation. We tend to see the more commonly used term "docblock".

I don't believe it's accurate to say we're duplicating documentation in PHP's docs in this patch. They are only referenced in @see lines. This patch is rectifying the lack of any docblocks for the methods in PhpStreamWrapperInterface.

I've updated the title and issue summary to use the word "docblock" in place of "phpdoc" to try and clarify this.

I agree this is necessary because PHP doesn't provide an interface itself here, although end users of this subsystem are expected to conform to the interface all the same. If we document them then IDE users that are developing stream wrappers will get helpful documentation about these methods,

As part of this patch we can also remove the duplicated (but less detailed) docblocks in LocalStream and replace them with @inheritdoc.

+++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
@@ -10,37 +10,142 @@
+   * @see streamWrapper::dir_readdir()
...
+   * @see streamwrapper::rmdir()
...
+   * @see streamwrapper::mkdir()
+   * @see streamwrapper::unlink()

@@ -65,21 +170,76 @@ public function stream_cast($cast_as);
+   * @see streamWrapper::dir_closedir()

@@ -183,12 +401,30 @@ public function stream_seek($offset, $whence = SEEK_SET);
+   * @see streamwrapper::url_stat()
...
+   * @see streamWrapper::stream_tell()

@@ -202,21 +438,85 @@ public function stream_tell();
+   * @see streamWrapper::rmdir()
...
+   * @see streamwrapper::stream_stat()

I don't think any of the "@see streamWrapper::" lines are useful as we don't have such a class in core, they seem to be referring to the original PHP example docs. We should either refer to methods on *this* interface or remove these.

I don't believe it's accurate to say we're duplicating documentation in PHP's docs in this patch.

The task IS does say to copy the docs what is implemented and what is implemented is the PHP Stream Wrapper class. Also this review is clearly referred to the documentation for www.php.net.

I am confused. Maybe if I work on the patch it will make sense.

I have updated the patch with the suggestions in #22. I have also moved comments that were after the @return to the top part of the docblock.

No, still confused. :-)

Looking good, just a couple of points:

1. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -384,39 +307,17 @@ public function unlink($uri) {
   +    [$scheme] = explode('://', $uri, 2);
   

   This change is out of scope (PHPStorm likely did this automatically).

2. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
   @@ -10,37 +10,144 @@
   +   * 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().
   

   Unsure what we should do to these references to streamWrapper, as they are from the original docs. I suppose we should just replace them with this interface?

3. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
   @@ -10,37 +10,144 @@
   +   * Note, the streamWrapper::$context property is updated if a valid context is
   

   This reference is more tricky because $context is expected to exist on the stream wrapper object, but interfaces can't define properties.

1. Fixed
2. Yes, these are from the original docs. I changed them to PhpStreamWrapperInterface::
3. The only idea I had was to add a comment in the class doc bloc but I couldn't think of any text!

Thanks for the update. I reviewed the interdiff and have some feedback:

1. The PhpStreamWrapperInterface changes seem okay.

2. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -2,6 +2,8 @@
   +use org\bovigo\vfs\vfsStreamWrapper;
   +
   

   I'm unclear why this was added.

3. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -416,6 +418,7 @@ public function dir_rewinddir() {
   +    $a = new vfsStreamWrapper();
   

   Ditto.

4. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -89,7 +91,7 @@ protected function getTarget($uri = NULL) {
   -    list(, $target) = explode('://', $uri, 2);
   +    [, $target] = explode('://', $uri, 2);
   

   #24.1 is referencing the code below but the interdiff has the above, so this confused me.

   +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
   @@ -384,39 +309,17 @@ public function unlink($uri) {
      public function dirname($uri = NULL) {
   -    list($scheme) = explode('://', $uri, 2);
   +    [$scheme] = explode('://', $uri, 2);
   

@Kristen Pol, thnaks.

1. Yay!
2. I don't remember why I added that, removed now.
3. This time I really have removed all accidental format changes.
4. Ah, I must have been playing around with a test. It is removed.

Yes, I forgot to run commit-code-check.

Thanks for the update.

1. Looks like everything from #26 is covered by #27
2. "Spelling" changes seem okay in #28
3. Tests pass
4. Manual testing is not needed as this is documentation
5. The only thing I'm unclear about is #24.3 so leaving open for review

I don't have any better suggestions about the $context property, as this is copied from the PHP docs I don't think there is much else we can say or do. Other than the nits below this looks ready to go to me.

1. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
   @@ -116,12 +279,73 @@ public function stream_lock($operation);
   +   * Note the `streamWrapper::$context` property is updated if a valid context
   

   We don't usually use Markdown-style backticks in core, this is slightly different from the other instances of this note.

2. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
   @@ -116,12 +279,73 @@ public function stream_lock($operation);
   +   *   delimited by `://` are supported. `:` and `:/` while technically valid
   

   Same here, we should use double quotes probably.

3. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
   @@ -202,21 +444,86 @@ public function stream_tell();
   +   *   a `://` delimited URL. Other URL forms are not supported.
   

   And here

1. Wasn't sure if you wanted double quotes here. I have just removed the back ticks.
2 and 3. Now use doube quotes..

Thanks, this looks good to go.

Backported to 9.2.x as this only changes documentation.

Committed and pushed ca2c3d48a3 to 9.3.x and 5831c967c7 to 9.2.x. Thanks!