Fully conform to PHP5.4 streamwrapper class

Some code for tests:

  $fileName = "public://test.txt";
  file_put_contents($fileName, "line of text\n");
  chmod($fileName, 0644);
  $ch = curl_init();
  $fh = fopen($fileName, 'r');
  curl_setopt($ch, CURLOPT_INFILE, $fh)

Results in:

• Warning: chmod(): DrupalPublicStreamWrapper::stream_metadata is not implemented! in ...
• Warning: curl_setopt(): DrupalPublicStreamWrapper::stream_cast is not implemented! in ...
• Warning: curl_setopt(): DrupalPublicStreamWrapper::stream_cast is not implemented! in ...
• Warning: curl_setopt(): cannot represent a stream of type user-space as a STDIO FILE* in ...

And these are not warnings that can be ignored: the chmod is not performed and a curl_exec() will really fail later on.

Another issue to decide on: should stream wrappers clear the statcache or is that the responsibility of the using code. See comments #29 to #34 of parent issue #2107287: PHP 5.4 calls a new stream_metadata() method on stream wrappers not implemented by Drupal.

This is a review of the changes to interface PhpStreamWrapperInterface only. I will try to review the rest later.

1. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
   @@ -13,16 +13,82 @@
   +   * @see stream_select()
   +   * @see http://php.net/manual/streamwrapper.stream-cast.php
   

   The "see also" section is not copied on the other methods added (e.g. @see fseek on stream_seek()). Shall we leave that section out everywhere and only add the @see with a link to the copied doc on php.net?

2. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
   @@ -13,16 +13,82 @@
   +  /**
   +   * @return void
   +   */
      public function stream_close();
   

   Do we document this as such in Drupal?

I finally got some time to further review...

LocalStream:
If LocalStream is meant as the base class for all "Drupal special locations" wrappers (public, private, temp, and, yet in contrib, module, theme, library) then it should fully implement the PhpStreamWrapperInterface with real implementations, no "silent failures" by just returning false. IMO that is what this issue is all about.

Thus the patch should contain "real implementations" for:
- stream_cast: return $this->handle (see e.g. comment #1).
- stream_set_option: I think it should call one of: stream_set_blocking(), stream_set_timeout() or stream_set_write_buffer().
- stream_truncate: it should call ftruncate($this->handle).
- stream_metadata should also handle the options PHP_STREAM_META_OWNER_NAME, PHP_STREAM_META_OWNER, PHP_STREAM_META_GROUP_NAME, PHP_STREAM_META_GROUP.

Adding real functionality, also means writing new tests or adapting existing ones that just tested that false was returned (if these exist). We can do so method by method in separate child issues, but I don't think it will be to much for 1 issue.

ReadOnlyStream:
What is the use of this class? It is not subclassed in core, PHPStorm not even finds a dummy test class that derives from this! Could this be some kind of historical left over, made redundant by (the IMO better named) LocalReadOnlyStream?

I think that we can remove this class!

LocalReadOnlyStream:
This class should also override methods stream_cast and stream_set_option and trigger an error, like is done with stream_truncate. I guess there should be tests for these as well (including stream_truncate).

In general:
- I would like to see the same method order in LocalStream and LocalReadOnlyStream as in PhpStreamWrapperInterface, being the order as documented on php.net, but this is better done in a child issue.

If I find some time, I will give it a try but will first assign the issue to me then. So, in the mean time, everyone, feel free to assign it to you and give it a try.
- For stream_cast I have a "test"/expected behavior in #1
- stream_set_option: let's find out.
- stream_metadata: if options cannot or should not be supported we need to trigger an error (I thought that chown/chgrp can be done by the owner of a file without further rights, but I am no *nix expert).
- ReadOnlyStream: created #2236163: Remove class ReadOnlyStream (or turn into a trait?).
- LocalReadOnlyStream: should be done in the next patch.

In addition to patch of #15:
- Some tests.
- Solved an error: stream_set_option() on read-only stream should only fail for option STREAM_OPTION_WRITE_BUFFER.

I am still not sure what to do with ReadOnlyStream. Implementing stream_cast() should be easy, but implementing stream_set_option() is difficult without knowing that there is a parent class that implements it full blown, even if we would do that in a trait that would be a problem.

OK, I can't get any of the options in stream_set_option to work, at least not on Windows, even not on standard local files using the mentioned functions stream_set_timeout(), etc. If you search on the internet, there is some mentioning of this, but it is nowhere explicitly stated that it indeed does not work with local files on Windows.

As these are advanced use cases anyway, I suggest to not implement it on our local stream wrappers and fail with a clear error message saying so.

- the PHP core function stream_set_write_buffer() returns -1, not false.
- improved some comments.

I reviewed the parts that sun provided (in #15) and can give an RTBC on that.

If someone else can review my changes made on top of #15, we can progress. To that end, I tried to create some kind of interdiff between the patches in #15 and #20. In short, the differences between #15 and #20:
- Added some tests for those methods that got a real implementation.
- Undid the implementation of stream_set_option() including the override in the local read-only stream, as it could not be made to work (on Windows). Note that this implementation was added by sun on my request, but now - based on testing - I propose to not implement it any more.

Another note: ReadOnlyStream is not kept up to date with the changes, as we might remove that class (or update it in accordance with this issue) in #2236163: Remove class ReadOnlyStream (or turn into a trait?).

Rerolled, let's see if this patch does apply. also updated the issue summary.

Issue #1676910: Rename randomName() to randomMachineName() renamed method TestBase::randomName to randomMachineName...

Let's try to activate the testbot.

Updated the issue summary as per #31. Most noteworthy change:

The actual API changes are minimal and exists of additions only: addition of some methods and addition of some optional parameters.

However, stream wrapper methods are normally not called directly, but via the standard available PHP file functions (fread, ftruncate, chmod, etc.). So the importance of this issue is not in the API changes but in correcting a broken file handling system.

RE #33:
In that sense you are right, however:

• If it is a wrapper around the local file system, they should better extend Local(ReadOnly)Stream (renamed in D8, was DrupalLocalStreamWrapper in D7) in which case they get implementations of the new methods for free.
• If it is a wrapper around a remote service or file system, they should implement these methods themselves anyway to not get the warnings and silent failures as mentioned in the issue summary and #1. Remember, it is not us that made these changes but PHP 5.4.

Known contrib modules:

• System stream wrapper: has a patch for being added to D8 core, but that patch might not make it anymore. But with that patch, a port will be trivial and as it does extend LocalReadOnlyStream, there won't be any problems.
• Remote stream wrapper: no D8 port yet.
• Amazon S3: no D8 port yet

So, I don't see foresee any problems.

I hope to have clarified the reasons behind and the importance of this issue, so please review, so that a core maintainer can have its say on it.

Thanks for reviewing.

re #35.1: done, I just renamed the test method to one that better describes what is tested.

re #35.2: I think that users/overriders should be able to find out what a method does by only looking at the docs, thus without a need to read the code itself, even when that is available. So I left it as is, but did made a few changes to get a 1 line short description and removing the "we/you".

re #35.we/expand/unify/remove:
There already is a follow-up specific for the documentation: #2228087: PhpStreamWrapperInterface lacks docblocks. So
- I left the other usages of we, as they were already there.
- I left the "@return int" doc blocks, see comment #9.2.
- I left the @see tags, as they come from the PHP manual.
- I left any duplication, as there is a follow-up.

Though not completely following your suggestions, I hope that with my reasoning behind that, you can accept this patch as is.

According to the Drupal documentation guidelines an {@inheritdoc} must be the only line in a docblock (https://www.drupal.org/node/1354#inheritdoc), even when the phpdoc site does describe the construct you mention (but does allow so only for class docblocks...). As I also prefer to augment rather then repeat and extent, I go with your suggestion as well as the rephrasing (but augmented that one with specifically mentioning the return value false and the triggered error).

@alexpott: thanks for looking at it.

re #40.1: I updated the issue summary. I hope I understood the template correctly and this is what you were looking for.

re #40.2: This is a property of the proposed PHP stream wrapper class and thus what we should implement. However, as properties do not belong to interfaces, I removed it from the patch. Whether the local stream should implement it is unclear to me. Looking at where contexts are used, I only see http, ftp, curl and such usages, no local stream wrappers. Moreover, no errors or warnings are filed in the issue queue about PHP complaining about this.

re #40.3: I can do it here, but would like to postpone it to the child issue #2228087: PhpStreamWrapperInterface lacks docblocks (also see comment #36).

@alexpott: thanks for committing. I hope to find some time to update the documentation as per #2228087: PhpStreamWrapperInterface lacks docblocks.

Regarding the ReadOnlyStream: IMO that's a class that should not be there, therefore I started: #2236163: Remove class ReadOnlyStream (or turn into a trait?).