Add an abstract DrupalReadOnlyStreamWrapper that other stream wrappers can extend

Attached patch adds two new classes:

1. ReadOnlyStream - an abstract implementation of StreamWrapperInterface which defines the write-related functions.

2. LocalReadOnlyStream - an abstract extension of LocalStream which extends the write-related functions to not actually write.

It made sense to me to extend LocalStream with it's own read-only class in order to have the least amount of code duplication.

hmm... my IDE didn't pick the missing bracket up. My bad - new patch with correct syntax up.

Just updating to note that the LocalReadOnlyStream class from the above patch is also used in the patch in #6 from #1308152: Add stream wrappers to access extension files.

Ideally, these two issues should be committed at the same time, with the overlapping class removed from the other patch.

Probably should tag this one too.

@xjm: Can you add a comment explaining why Views needs this?

[Edit: never mind. I see you did that in #1308152-18: Add stream wrappers to access extension files. thanks]

Patch looks good aside from No newline at end of file and applies cleanly to latest 8.x.

Added a note in #1308152: Add stream wrappers to access extension files that this issue to creates /core/lib/Drupal/Core/StreamWrapper/LocalReadOnlyStream.php along with /core/lib/Drupal/Core/StreamWrapper/ReadOnlyStream.php which reflects the sentiments of #5

#4: 1308054-ReadOnlyStream-LocalReadOnlyStream-classes-rev2.patch queued for re-testing.

Is there a reason to not move ahead with this? Patch is green, offers some nice baseline utilities, and there's a patch lined up that would benefit form it.

Questions/remarks:

• Why don't we throw exceptions on forbidden methods?
• If we don't throw an exception, we should log/show an E_WARNING message (as is kind of indicated by http://www.php.net/manual/en/stream.errors.php).
• stream_lock() with LOCK_EX should be forbidden?
• stream_open() with a mode different from 'r' should be forbidden?
• stream_flush() should be forbidden?
• stream_truncate() should be forbidden?

For an implementation of (most of) the points above see my own (not perfect) D7 imagecache_actions implementation of a module:// wrapper. I did not compare it to Dave Reid's D7 version yet.

The specific functions/methods we're overriding here specify that they should return FALSE or TRUE or 0 or whatever in certain cases that we're emulating. We're emulating those, so "0 bytes got written, sry" seems like the appropriate return. Raising E_WARNING is really hard to test, debug, trap, etc. That definitely is a bad idea.

Needs review for the other methods that need checking to see how they should be handled, per #12.

IMO, error handling is part of the "interface". In this case no exceptions are thrown so forget that. But I think that a side effect in the form of a warning should be added as silent failure is even a worse idea.

Except that triggering glbal errors is a horrible way to do error handling, which is why it's strongly discouraged in favor of exceptions these days. Let's not do that.

Taking a stab at this one.

Not tested, but here's a first crack.

- getTarget() indicates that it returns a writeable destination. I didn't override this particular function, assuming that the subsequent fopen($file, 'w') failing would be sufficient.
- Added support for stream_open, which returns FALSE on anything but $mode == 'r'.
- Added support for stream_lock, disallowing LOCK_EX or LOCK_EX | LOCK_NB.
- Added support for stream_flush, disallowing flushing of output to storage. The LocalStream version could return TRUE if there was no data to flush, bit I'm returning FALSE on all attempts even if there was no data to flush.
- stream_truncate() does not exist in the current LocalStream implementation (or it's parents), so no need to override it at this time.
- Added a DummyReadOnlyStreamWrapper class (which extends LocalReadOnlyStream) for use with testing, and added it to hook_stream_wrappers() in file_test.module.
- Added a ReadOnlyStreamWrapperTest.php file, which tests the write functions of the DummyReadOnlyStreamWrapper class.

Now with fewer mislabeled docblocks!

- stream_read() should trigger a E_WARNING when STREAM_REPORT_ERRORS is set, as specified in the PHP documentation.
- stream_lock() should be disallowed. Locking only makes sense for writable streams. It should trigger a E_WARNING, as specified.
- unlink() should trigger a E_WARNING, as specified.
- rename() should trigger a E_WARNING, as specified.
- mkdir() should trigger a E_WARNING, as specified.
- rmdir() should trigger a E_WARNING, as specified.
- StreamWrapperInterface::realpath() and StreamWrapperInterface::dirname() needs to be implemented too.

The current drupal\core\lib\Drupal\Core\StreamWrapper\PhpStreamWrapperInterface.php file contains an "old" version of the example class in http://www.php.net/manual/class.streamwrapper.php. This means that there is actually no single defined interface class at all for streamwrappers in PHP.

#17: If we say that the current interface suffices for (all) our purposes, we indeed do not have to implement stream_truncate(). If we say we want to implement all of the new interface, I think we better open a separate issue for that, because that will affect all existing stream wrapper implementations as well.
#20: I think that realpath() should remain abstract, the derived classes (module://, theme://, etc.) will have to implement that

I meant that they need to be implemented in the concrete class DummyReadOnlyStreamWrapper.

Agreed that stream_truncate() would be a different issue ... my original post actually stated that 'if we decide to add it to LocalStream, it will have to be added here too'; but I lost the text - #17 is an abbreviated version.

- stream_read() should trigger a E_WARNING when STREAM_REPORT_ERRORS is set, as specified in the PHP documentation.

This patch simply duplicates the logic in LocalStream, and should behave consistently with the parent class. If we want LocalStream to trigger an E_WARNING, I see that as a new issue (since this is simply dealing with the abstract class). EDIT: On second glance, the existing LocalStream implementation will probably trip the E_WARNING during the fopen() call ... so my comment is invalid. If the caller is explictly specifying STREAM_REPORT_ERRORs, then raising a warning may be reasonable.

- stream_lock() should be disallowed. Locking only makes sense for writable streams.

What about the case where multiple scripts are accessing the same file via two different schemes? (i.e. Is there any valid situation where a script may have a file/stream open as writeable, and another has it opened via a read-only wrapper? Just a question, as I've never worked with stream wrappers before this patch.)

...It should trigger a E_WARNING, as specified.
- unlink() should trigger a E_WARNING, as specified.
- rename() should trigger a E_WARNING, as specified.
- mkdir() should trigger a E_WARNING, as specified.
- rmdir() should trigger a E_WARNING, as specified.

#13 suggested that E_WARNING was a bad idea ... again, I've never worked with stream wrappers before this patch, so I'll leave that debate up to others to resolve.

- StreamWrapperInterface::realpath() and StreamWrapperInterface::dirname() needs to be implemented too.

I'll look at adding these to the DummyReadOnlyStreamWrapper class tonight, if it isn't picked up by someone else before then.

PHP specifies that streamwrapper implementations must throw E_WARNING in case of error or unimplemented methods. It's not like you actually have a choice.

#26: stream_lock should only be disallowed only when it wants to obtain an exclusive lock (LOCK_EX), shared locks are OK.

PHP specifies that streamwrapper implementations must throw E_WARNING in case of error or unimplemented methods.

Doh! It certainly does!

DummyReadOnlyStreamWrapper->realpath() and dirname() need to be implemented too.

These are both implemented in LocalStream, and are thus inherited up the LocalStream->LocalReadOnlyStream->DummyReadOnlyStreamWrapper extends chain. I assume this should be sufficient ... or are there some changes to the logic that need to be added here?

Test is not passing locally yet, but not able to spend any more time on this for the moment ... here's the changes to date.

Changing status for bot.

I'm going to assume it's okay to simply mask out the E_WARNINGS in the test ...

Minor documentation fix. Otherwise identical to #33.

class LocalReadOnlyStream

stream_open:

+    if ((bool) $this->handle && $options & STREAM_USE_PATH) {

& has higher precedence than &&, so I think this expression is incorrect, if not it would be better to add some documentation. I also prefer to explicitly check on FALSE instead of casting file resources or whatever to bool (thus: this->handle !== FALSE)

stream_lock:

+   *   stream wrapper.  Return TRUE for all other valid $operations.

It actually returns the result of flock() for all other operations, thus FALSE when, e.g, the lock could not be acquired.

+    if (in_array($operation, array(LOCK_EX, LOCK_EX | LOCK_NB))) {

I guess those are bit masks, thus some bitmasking logic seems easier and more appropriate.

+    if (in_array($operation, array(LOCK_SH, LOCK_UN, LOCK_NB))) {

IMO, other checking on bit masks is not up to this class, so just return the result of flock() and let flock() do the checking on non-existing operations.

chmod:

+   * Implements Drupal\Core\StreamWrapper\StreamWrapperInterface::chmod().

Seems so obvious to me, plus that it is not mentioned with other methods. So this line can be removed.

class ReadOnlyStream

The same remarks.

+    if ((bool) $this->handle && $options & STREAM_USE_PATH) {

Changed to if ($this->handle !== FALSE && ($options & STREAM_USE_PATH)). The extra brackets are extraneous, but they help illustrate that the & STREAM_USE_PATH is just a bitmask check on $options.

+   *   stream wrapper.  Return TRUE for all other valid $operations.

Updated to Return the result of flock() for other valid operations. Defaults to TRUE if an invalid operation is passed. I can't explain why the return TRUE is there ... I left it in to provide consistency with the existing LocalStream.php operation (which this class extends).

+    if (in_array($operation, array(LOCK_EX, LOCK_EX | LOCK_NB))) {

+    if (in_array($operation, array(LOCK_SH, LOCK_UN, LOCK_NB))) {

flock() is an odd-ball, in that the PHP docs suggest only the LOCK_NB can be applied as a bit mask. If the former values of the constants listed on the flock() PHP page are correct, then LOCK_SH|LOCK_EX is equal to LOCK_UN ... thus normal bitmask-checking doesn't work here.

Which means, of course, that the second in_array() check here was invalid ... I've updated it to (LOCK_SH, LOCK_UN, LOCK_SH|LOCK_NB).

IMO, other checking on bit masks is not up to this class, so just return the result of flock() and let flock() do the checking on non-existing operations.

I've left in the check to provide consistency with the LocalStream.php implementation that this is extending ... mostly because of i) the potential of int values which might be interpreted as LOCK_EX by flock()'s internal code, and ii) the unknown motivation behind the return TRUE; at the end of LocalStream.php's implementation (and what might break if that were removed).

Note: The php documentation of ::stream_lock() and flock() seem to differ. The existing LocalStream.php reflects the ::stream_lock() version; which might infer that $operation can contain a standalone LOCK_NB value ... which I understand to be incorrect.

Great, I accept your reason for not removing the other "bit mask" testing. So RTBC for me. Do we need an additional reviewer to really set it to RTBC?

Per #11 and #37: RTBC. This can yet go into D8.

I'm not sure about the system stream wrappers which is the main core use-case for this, but it's not doing any harm adding this to remove code duplication elsewhere regardless of how that issue goes, so committed/pushed to 8.x. Needs a change notice for the API addition.

Preliminary change record at http://drupal.org/node/1886974.

My only question with the change notice, which may already be covered elsewhere: If I have a read-only stream, why would I use local vs. not?

If that's covered elsewhere, we can close this. If not, let's add it and then close this. :-)

Added "The LocalReadOnlyStream abstract class additionally provides default implementations for reading files so that extending stream wrappers only need to define the directory and file structure.". Good enough? :)

Edit: Adjusted it a bit more and added fully namespaced names of the two classes.

I filed issue #1954180: LocalReadOnlyStream::stream_open refuses to open a file when 'rb' flag is passed in that concerns the code that got committed with this issue. Can someone of the followers review the patch over there? It is just a few lines and some changes to the tests.

Thanks

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

I would like to invite you to give your opinion on #2236163: Remove class ReadOnlyStream (or turn into a trait?). Thanks in advance for your reactions.

Fix media tag.