Problem

  • PHP 5.4 calls a new stream_metadata() method on stream wrappers for chmod(), touch(), chown(), and chgrp() operations, while PHP 5.3 does not, which causes our stream wrappers to blow up depending on PHP version.
  • We largely didn't notice, because most of our chmod() calls in core are @error-suppressed.

Proposed solution

  1. Drop the custom StreamWrapperInterface::chmod() method and require all stream wrappers to implement stream_metadata() instead.

Original report

Running D8 tests on my local PHP5.4 installation I get tons of this warning (especially on cleaning up after a failed test):

Warning: chmod(): Drupal\Core\StreamWrapper\PublicStream::stream_metadata is not implemented! in Drupal\simpletest\TestBase::filePreDeleteCallback() (line 1339 of core\modules\simpletest\lib\Drupal\simpletest\TestBase.php).

A bit bizar that chmod warns about it, but as of PHP5.4 stream_metadata is part of the suggested StreamWrapper class: http://php.net/manual/en/streamwrapper.stream-metadata.php.

We don't require PHP5.4 yet, but we do support it. So this warning should be prevented. I guess we can do some dummy implementation if it is not really used elsewhere.

Looking at the suggested list of methods in the 5.4 documentation, I see 4 missing methods:
- public bool stream_metadata ( string $path , int $option , mixed $value )
- public bool stream_truncate ( int $new_size )
And already part of PHP5.3:
- public resource stream_cast ( int $cast_as )
- public bool stream_set_option ( int $option , int $arg1 , int $arg2 )

I'm not sure if they all may lead to a warning somewhere, but I think we better implement them to prevent warnings popping up somewhere sometime.

CommentFileSizeAuthor
#82 interdiff-2107287-76-82.txt1.13 KBBR0kEN
#82 drupal-stream-wrapper-php54-2107287-82.patch5.91 KBBR0kEN
#76 interdiff-2107287-74-76.txt737 bytesBR0kEN
#76 drupal-stream-wrapper-php54-2107287-76.patch5.91 KBBR0kEN
#74 interdiff-2107287-71-74.txt1.11 KBBR0kEN
#74 drupal-stream-wrapper-php54-2107287-74.patch5.86 KBBR0kEN
#71 interdiff-2107287-67-71.txt1.4 KBBR0kEN
#71 drupal-stream-wrapper-php54-2107287-71.patch5.38 KBBR0kEN
#67 interdiff-64-67.txt274 bytesBR0kEN
#67 drupal-stream-wrapper-php54-2107287-67.patch5.34 KBBR0kEN
#64 interdiff-2107287-60-64.txt8.8 KBBR0kEN
#64 drupal-stream-wrapper-php54-2107287-64.patch5.34 KBBR0kEN
#60 drupal-stream-wrapper-php54-2107287-60.patch6.35 KBBR0kEN
#54 2107287-54.patch3.01 KBNeograph734
#51 interdiff.txt716 bytesNeograph734
#51 2107287-51.patch2.98 KBNeograph734
#49 interdiff.txt1.84 KBNeograph734
#49 2107287-49.patch2.79 KBNeograph734
#45 2107287-45.patch1.5 KBNeograph734
#41 2107287-7-41.patch3.1 KBNeograph734
#39 2107287-7.patch2.88 KBNeograph734
#30 interdiff.txt603 bytessun
#30 stream.metadata.30.patch7.96 KBsun
#27 interdiff.txt1.01 KBsun
#27 stream.metadata.27.patch7.81 KBsun
#22 interdiff.txt1.98 KBsun
#22 stream.metadata.22.patch7.66 KBsun
#9 drupal8.stream-metadata.9.patch7.66 KBsun
#8 drupal8.stream-local-metadata.8.patch2.08 KBsun
#3 2107287-3.patch1.78 KBamateescu
#3 interdiff.txt621 bytesamateescu
#1 2107287.patch1.7 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Title: Implement new PHP5.4 stream wrapper methods. » Implement the new stream_metadata() stream wrapper method from PHP 5.4
Component: file system » base system
Status: Active » Needs review
Issue tags: +PHP 5.4
FileSize
1.7 KB

I'm not sure we should implement all the new methods blindly, let's just fix issues as they come.

This simple patch does it for the warning in the OP.

fietserwin’s picture

Status: Needs review » Needs work

Thanks for joining this issue.

  1. Interface PhpStreamWrapperInterface should be extended as well. Including the method documentation. Perhaps the interface documentation should make clear that this is the 5.3 interface and that as soon as we require PHP5.4 (D9?) this will be outdated.
  2. abstract class ReadOnlyStream does not inherit from LocalStreamWrapper so, should also get the same method (with an @inheritoc).

Looking a bit more to the documentation, I think we won't get away with a dummy implementation. I think that, in the case of the warning from the OP, we really should do a chmod on the underlying resource.

Updated the issue summary, 2 methods I mentioned are already suggested for PHP 5.3.

amateescu’s picture

Status: Needs work » Needs review
FileSize
621 bytes
1.78 KB

That's what I tried to say in #1, the interface does not need to be extended. For example, LocalStream has a stream_cast() implementation which is not in the interface.

ReadOnlyStream does not need to implement this method because, as the name says, it's read only, so you can't call chmod() on it. If you check the implementation, it even triggers an error if you try to.

Also, you somehow read the docs wrong, stream_metadata() is called *in response* to a chmod(), see the possible values of the $option param. So, really, we just need to return false :)

I added a @see to match all the other methods, and I still think this patch is all we need to do.

fietserwin’s picture

Status: Needs review » Needs work

After posting, I indeed also saw that the interface and implementations do differ in what part of the suggested list of methods they implement.

*In response to* is indeed a bit vague. So I ran a simple test:

    file_put_contents("d:/tmp/d8.log", "testChmod54() begin\n", FILE_APPEND);
    $file = "public://favicon.ico";
    chmod($file, 0444);
    file_put_contents("d:/tmp/d8.log", "testChmod54() end\n", FILE_APPEND);

and adapted the methods chmod and stream_metadata of Localstream to also do some logging:

  function chmod($mode) {
    file_put_contents("d:/tmp/d8.log", "LocalStream::chmod($mode) begin\n", FILE_APPEND);
    $output = @chmod($this->getLocalPath(), $mode);
    // We are modifying the underlying file here, so we have to clear the stat
    // cache so that PHP understands that URI has changed too.
    clearstatcache(TRUE, $this->getLocalPath());
    file_put_contents("d:/tmp/d8.log", "LocalStream::chmod($mode) end\n", FILE_APPEND);
    return $output;
  }

  public function stream_metadata($uri, $option, $value) {
    file_put_contents("d:/tmp/d8.log", "LocalStream::stream_metadata($uri, $option, $value)\n", FILE_APPEND);
    return FALSE;
  }

To my surprise (and yours as well I guess) this is what I found in the log file:

testChmod54() begin
LocalStream::stream_metadata(public://favicon.ico, 6, 292)
testChmod54() end

So I think we have a real bug here and a nasty difference between PHP 5.3 and 5.4.

Regarding changing the interface or not: I favor completing the interface, albeit for clarity and explicitness. OTOH, if we would have implemented a silent failure (return FALSE) we probably would not have discovered this bug.

So, yes, change the interface, but in the base classes fail with a trigger_error as read only streams currently already do for non read operations. But we can split that off into its own issue.

amateescu’s picture

I'm sorry but I don't see what the bug is :)

fietserwin’s picture

I would have expected that when the function chmod() is called with a file name in stream wrapper syntax, eventually the method chmod() of the stream wrapper would be called. It is not, it is stream_metadata() that gets called.

So I tihnk that chmod("public://favicon.ico", 0444); is a no-op when we don't really implement stream_metadata().

amateescu’s picture

Hm.. ok, that's weird. Unfortunately, I'm not so versed in stream wrappers so I can't say how to proceed :/

amateescu’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Component: base system » file system
Issue summary: View changes
Priority: Normal » Major
Status: Needs work » Needs review
Related issues: +#2176141: Add a return value to file_save_htaccess()
FileSize
2.08 KB

Let's move the other missing methods into a separate issue, please.

Attached patch properly implements the touch() and chmod() stream metadata methods.

The chown() and chgrp() methods are not supported (or should not be supported), because they require super-user permissions. Drupal's PHP process generally should not have super-user permissions.

This fix blocks me from completing my tests for #2176141: Add a return value to file_save_htaccess(). Also, this is at least major.

I don't know how to test this. I'd think that existing local file usage throughout core should be sufficient.

sun’s picture

Assigned: Unassigned » sun
FileSize
7.66 KB

Ok, here's a more complete patch.

The stream_metadata() method makes our custom StreamWrapperInterface::chmod() method completely obsolete.

Our StreamWrapperInterface did not support touch(), so that requires no further changes (but is supported now). chown() and chgrp() were never supported, most likely for the reasons I mentioned before.

The impact of this single new method clarifies why it's a good idea to have a dedicated issue for each of the new PHP stream wrapper methods.


Now I just hope that the testbot won't choke, because it still runs PHP 5.3.......

Status: Needs review » Needs work

The last submitted patch, 9: drupal8.stream-metadata.9.patch, failed testing.

sun’s picture

Title: Implement the new stream_metadata() stream wrapper method from PHP 5.4 » PHP 5.4 calls a new stream_metadata() method on stream wrappers not implemented by Drupal
Priority: Major » Critical
Issue summary: View changes

Clarifying the true extent and scope of this issue. Based on that, bumping to critical.

What's the status of upgrading testbots to PHP 5.4?

fietserwin’s picture

Status: Needs work » Needs review
  1. +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
    @@ -23,6 +23,39 @@ public function stream_seek($offset, $whence);
    +   * @return bool
    +   *   Returns TRUE on success or FALSE on failure. If $option is not
    +   *   implemented, FALSE should be returned.
    +   *
    

    Let's formalize in the interface (the "contract") that trigger_error() should be called if an option is not supported/implemented. This text should not be in the @return tag (it specifies a side effect,) but in the method description (above the @param's):

    If an option is not implemented trigger_error() should be called with at least a level of E_USER_WARNING and FALSE should be returned.

Setting to needs review to have the testbot test it again.

fietserwin’s picture

9: drupal8.stream-metadata.9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: drupal8.stream-metadata.9.patch, failed testing.

fietserwin’s picture

Now we have real feedback from the test bot: all chmod() (and thus drupal_chmod()) calls on file handles that link to a wrapped resource fail. This is unsolvable as long as the test bots don't run PHP5.4 (which is now a requirement for D8).

Alternatively we could implement a solution that supports both 5.3 and 5.4 but a test bot won't test the 5.4 path. That solution would be:
- keep method chmod() in wrappers. its code should be the code in the "case STREAM_META_ACCESS:" branch in the patch.
- change that case branch in stream_metadata to call the chmod() method.

I locally tested all tests under File, File API, file API (remote), and Image (thus not PHPUnit test). I had 7 errors on file download, see below, but all the errors the test bot reported passed.

  • Confirmed that the generated URL is correct by downloading the created file. Browser DownloadTest.php 42 Drupal\file\Tests\DownloadTest->testPublicFileTransfer() Fail
  • Generated URL matches expected URL. Other DownloadTest.php 143 Drupal\file\Tests\DownloadTest->checkUrl()
  • HTTP response expected 200, actual 404 Browser DownloadTest.php 152 Drupal\file\Tests\DownloadTest->checkUrl()
  • Generated URL matches expected URL. Other DownloadTest.php 143 Drupal\file\Tests\DownloadTest->checkUrl() Fail
  • Generated URL matches expected URL. Other DownloadTest.php 143 Drupal\file\Tests\DownloadTest->checkUrl()
  • HTTP response expected 200, actual 404 Browser DownloadTest.php 152 Drupal\file\Tests\DownloadTest->checkUrl()
  • Generated URL matches expected URL. Other DownloadTest.php 143 Drupal\file\Tests\DownloadTest->checkUrl()
fietserwin’s picture

I was doing some coding in a custom module for a D7 site running on PHP5.4 and got this warning:
Warning: curl_setopt_array(): DrupalPublicStreamWrapper::stream_cast is not implemented!
Trying to FTP a file from public to another server. So we should implement that one as well for our stream wrappers. But as @sun asks in #8 and #9, we better do so in a separate issue.

Should we open a separate issue for D7 as well? The solution there will be completely different.

fietserwin’s picture

fietserwin’s picture

Child issues do not have to be listed as a related issue...

fietserwin’s picture

Status: Needs work » Needs review

According to https://groups.drupal.org/node/413508, the test bot is on 5.4.

fietserwin’s picture

9: drupal8.stream-metadata.9.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 9: drupal8.stream-metadata.9.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.66 KB
1.98 KB

Fixed phpDoc of PhpStreamWrapperInterface::stream_metadata() to comply with Drupal documentation standards.

Please note that the entire phpDoc is taken over 1:1 from PHP's native StreamWrapper interface template. I do not plan to diverge from the original phpDoc.

I'm not able to reproduce the reported test failures locally, so trying again. (But then again, I'm also on Windows...)

Status: Needs review » Needs work

The last submitted patch, 22: stream.metadata.22.patch, failed testing.

fietserwin’s picture

I'm also on windows, so I can't debug the tests neither.

What I did however is reviewing the code to see where it might go wrong, given that the 2 reported asserts fail and other s thus passed). This lead me to the conclusion that @drupal_chmod($directory, 0444) returns false. However, the reason why is not clear to me. digging deeper into LocalStream, I saw that it heavily depends on its internal property $handle, without any check on that one being assigned a value:

- only stream_open() does give $handle a value.
- for stream_read(), stram_write() and functions like that it seems rather logical to expect $handle to have a value.
- for stream_stat() however this is not the case, it might be done on the stream wrapper without being opened first.
- stream_close() does not reset $handle to null.
- opening a directory cannot be done using fopen() (PHP5.4 on Windows) but should be done using opendir(). stream_open() only uses fopen().
- but even then, fstat() on a directory handle returns false (PHP5.4 on Windows)
- stream_stat() uses fstat() and thus needs a valid file handle, not a directory handle.

Test code I used for the above statements;

var_dump(fstat(null));

$f = 'd:\tmp';
var_dump(is_writable($f));

$fh = opendir($f);
var_dump(fstat($fh));

$fh = opendir($f);
var_dump(fstat($fh));

var_dump(stat($f));

Results of the above code:

Warning: fstat() expects parameter 1 to be resource, null given ...
bool(false)
bool(true)
bool(false)
bool(false)
array(26) { ...}

Conclusion: local stream wrapper is not usable for directories.

To solve:
- stream_stat should use stat() instead of fstat().
- stream_open should either use fopen() or opendir()
- stream_close should reset $handle
- Better checking on $handle having a value in stream_stat() (if fstat() is faster than stat() and therefore preferred over stat() if a handle is available).

I think that this will make the errors go away, even though I can't explain why the tests currently succeed giving the above, though is_writable() depends on stat() according to its docs.

Damien Tournoud’s picture

@fietserwin: ->stream_metadata has just be completely mis-named by the PHP developers. It should be simply ->metadata() because it doesn't act on the currently opened stream, but on a file path.

- stream_stat should use stat() instead of fstat().

No, it acts on a stream, and on a stream only.

- stream_open should either use fopen() or opendir()

No, PHP doesn't support fopen()-ing a directory in a portable manner. Most UNIX systems actually support that, but Windows doesn't.

Calling fopen() on a directory is therefore undefined, and we should not try to be smart about that.

- stream_close should reset $handle

No, it should not. Reading on a closed stream wrapper should do the same thing as reading on a closed file handle.

- Better checking on $handle having a value in stream_stat() (if fstat() is faster than stat() and therefore preferred over stat() if a handle is available.

No, for the same reason as above. stream_stat() is guaranteed to be called after stream_open(), any other case should act the same as a broken file handle.

fietserwin’s picture

OK, I missed the url_stat() method which does what I meant (working on a path, not a resource) and which is indeed called when is_dir() or is_writable() are called on a stream wrapper, so stream_stat() is not called at all in these tests here.

What else can go wrong?
Some tests, on windows ..., made me believe that perhaps the statcache should be cleared for $uri when chmod-ing the underlying $target:

      case STREAM_META_ACCESS:
        clearstatcache(TRUE, $uri);
        return chmod($target, $value);

However the old/current code, which this patch does not completely move from method chmod() to method stream_metadata(), does a clearstatcache() on $target, which I would expect the PHP system function chmod() to do?

sun’s picture

Status: Needs work » Needs review
FileSize
7.81 KB
1.01 KB

Added back clearstatcache().

I omitted that, because I did not understand why we'd have to explicitly clear it when the touch() and chmod() methods are executed on a local filesystem path... Shouldn't PHP already know?

In case this passes tests, I wonder whether we need to change the test to clear the stat cache manually instead?

sun’s picture

Hm. That was the trick.

However, I'm still not sure whether I agree with that clearstatcache(). Is that really the responsibility of the stream wrapper? If so, why?

fietserwin’s picture

If chmod() is not supposed to clear the stat cache, it is indeed the test that should be adapted.

The docs on clearstatcache() suggest that it is the responsibility of the user to clear the cache:

... you may want to clear the cached information. For instance, if the same file is being checked multiple times within a single script, and that file is in danger of being removed or changed during that script's operation ...

I interpret this as it is indeed the user's responsibility to not use the cache (thus clear it) when there is a danger of the file being modified during the script execution. Although subsequently it says that in case of calling unlink() the cache will be cleared, but nothing on other file functions.

sun’s picture

FileSize
7.96 KB
603 bytes

Alright, let's keep it, but let's ensure that it is properly documented as a convenience operation.

→ Added code comment to explain the clearstatcache() call.

sun’s picture

Let's move forward here?

fietserwin’s picture

2 small points, though IMO neither one is blocking, so I would be fine with RTBC'ing this one. Someone else?

  1. +++ b/core/includes/file.inc
    @@ -1344,18 +1344,8 @@ function drupal_chmod($uri, $mode = NULL) {
    -    if ($wrapper->chmod($mode)) {
    
    +++ b/core/lib/Drupal/Core/StreamWrapper/PhpStreamWrapperInterface.php
    +
    +  /**
    +   * Sets metadata on the stream.
    +   *
    +  ...
    +   *
    +   * @return bool
    +   *   Returns TRUE on success or FALSE on failure. If $option is not
    +   *   implemented, FALSE should be returned.
    +   *
    +   * @see http://www.php.net/manual/streamwrapper.stream-metadata.php
    +   */
    +  public function stream_metadata($uri, $option, $value);
    +
    

    Why copy all the docu from php.net? None of the other methods does have this. Let's decide on a standard:
    - copy docu from php.net like done here, but then for all methods
    - add a @see ...php.net... to all methods
    Whatever we decide on, I can do that for the other methods in the follow-up issue. But let's set the standard in this issue.

  2. +++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
    @@ -355,6 +344,34 @@ public function stream_cast($cast_as) {
    +      // For convenience clear the file status cache of the underlying file,
    +      // since metadata operations are often followed by file status checks.
    +      clearstatcache(TRUE, $target);
    +    }
    

    This doesn't seem to be the way normal file resource handling does, thereby making our stream wrappers not behave fully transparent.

    So, I'm more inclined to solving the test.

sun’s picture

  1. PHP core does not provide an actual StreamWrapperInterface.

    PHP core only defines a prototype/example StreamWrapper class implementation. This prototype/example implementation can change with newer PHP versions — e.g. in terms of available methods, expected parameters, or return values.

    Therefore, all of the methods on the PhpStreamWrapperInterface should have a copy of the phpDoc of the prototype/example class that is currently implemented.

    This patch only adds the phpDoc for the new/missing stream_metadata() method. Completing the phpDoc for the other methods is out of scope here.

    Created #2228087: PhpStreamWrapperInterface lacks docblocks

  2. I'd like to limit the scope of this issue to add the missing stream_metadata() method.

    The current LocalStream wrapper in HEAD performs the clearstatcache() after a chmod() operation. While I basically agree (as mentioned in previous comments), removing it is a change in expected behavior, which should be discussed separately.

twistor’s picture

Status: Needs review » Reviewed & tested by the community

Ugh. What a sad API. This looks correct.

fietserwin’s picture

+1 on RTBC.

The differences between versions make simply referencing to php.net indeed a bit tricky. For the other point, I added a remark to child issue #2213241: Fully conform to PHP5.4 streamwrapper class.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

OK I don't love the clearstatcache() either, but we can discuss that separately.

Also thanks for #2213241: Fully conform to PHP5.4 streamwrapper class.

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

  • Commit 35a33d6 on 8.x by catch:
    Issue #2107287 by sun, amateescu: PHP 5.4 calls a new stream_metadata()...
sun’s picture

Assigned: sun » Unassigned
Parent issue: » #2213241: Fully conform to PHP5.4 streamwrapper class

I'm not going to work on a backport, as I'll focus on getting #2213241: Fully conform to PHP5.4 streamwrapper class done for D8 instead. I'm also switching the parent/child relationship around, since that issue is pretty much a meta issue already.

Neograph734’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.88 KB

Oke, we've tried to port the patch back to D7, and this worked for us as of PHP 5.4. It needs testing on 5.3 though.

Status: Needs review » Needs work

The last submitted patch, 39: 2107287-7.patch, failed testing.

Neograph734’s picture

Status: Needs work » Needs review
FileSize
3.1 KB

Forgot to add a line to the Stream Wrapper Interface. If it fails it might be a PHP 5.3 incompatibility, but I can't test.

Status: Needs review » Needs work

The last submitted patch, 41: 2107287-7-41.patch, failed testing.

fietserwin’s picture

There are hard tabs in your patch, please use spaces according to the coding standards.

+++ b/includes/stream_wrappers.inc
@@ -185,7 +186,7 @@ interface DrupalStreamWrapperInterface extends StreamWrapperInterface {
-  public function chmod($mode);
+  // public function chmod($mode);
 

This function is used in PHP 5.3 Thus to o support both 5.3 and 5.4+ we need to implement both stream_metadata() and chmod (and in a later issue the other options in 2 ways as well). I think we best do so by not removing chmod() and only adding stream_metadata() in this patch. I thought about the one calling the other, but because chmod() does not accept an uri while stream_metadata() requires one, we better keep these functions separate. But let's document (in the interface) which one is for which PHP version.

Neograph734’s picture

I thought I had Notepad++ properly configured to deal with tabs and spaces, thanks for bringing it up.

This morning I came to the same conclusion, so I've only added stream_metadata().

Currently running a test on 5.3 to see if it behaves as expected. Will do 5.4 when it's done.

Neograph734’s picture

FileSize
1.5 KB

This looked good on local.

Neograph734’s picture

Status: Needs work » Needs review
Neograph734’s picture

Status: Needs review » Needs work

It just finished all file tests with PHP 5.4.16 on my local machine as well. fietserwin, could you do the documentation? I have no idea how to write phpdoc.

fietserwin’s picture

re #47:

+++ b/includes/stream_wrappers.inc
@@ -106,6 +106,7 @@ interface StreamWrapperInterface {
+  public function stream_metadata($uri, $option, $value);

Just copy the phpdoc from the patch in #30 over here, that will suffice.

Any reason why you removed the change to drupal_chmod() in file.inc between the patches in #41 and #45? I think it should always have been like this in the first place.

Tip: if you provide an interdiff, it is easier to monitor the changes from patch to patch. See e.g. http://www.drupal4hu.com/node/374.

Neograph734’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
1.84 KB

Ok, so here we go with documentation.

I initially removed it because I thought that was what sun did in #30 as well. After removing that, the managed file copy problems we were experiencing were gone, giving me confidence it had to be like that. After #42 and #43 I realised Drupal needs that function for other things as well.

fietserwin’s picture

Status: Needs review » Needs work

I am OK with the new patch, though one minor documentation detail:

+++ b/includes/stream_wrappers.inc
@@ -106,6 +106,37 @@ interface StreamWrapperInterface {
+  *
+  * @see http://www.php.net/manual/streamwrapper.stream-metadata.php
+  */

Let's document that this is PHP5.4 specific, something like adding:
This is a new method in PHP5.4: @see http://www.php.net/manual/en/migration54.methods.php. In PHP5.3 use the Drupal specific method DrupalStreamWrapperInterface::chmod().

Neograph734’s picture

FileSize
2.98 KB
716 bytes

Here you go

Neograph734’s picture

Status: Needs work » Needs review
fietserwin’s picture

RTBC, if you remove the tabs from your patch.

Tip: good IDE's do more than replacing tabs with spaces and such. They also do live syntax checking etc. Features that n++ will never get. There are good free IDE's around, e.g. Eclipse and Netbeans but PHPStorm is my favorite, free if you only do open source development with it.

Neograph734’s picture

FileSize
3.01 KB

I guess you meant the missing spaces in front of the phpDoc? Those were my mistake... Further there is no tab in the entire patch and formatting looks like it should right?

I've removed some trailing spaces and added some to the beginning of the documentation. Hope this did it.

I'll take a look at PHPStorm, thanks.

fietserwin’s picture

Status: Needs review » Reviewed & tested by the community

I was looking at the interdiff that does show tabs (especially when reviewing with dreditor).

But thanks for picking up this issue and porting it to D7. RTBC.

If the testbot for D7 will continue to run on PHP5.3 (I'm not sure about this), testing this code will be impossible as the code will never be called in PHP5.3. We could rewrite drupal_chmod() to take the PHP version into account in deciding whether to call the native chmod() or the DrupalStreamWrapperInterface::chmod(), but I am not sure that we want such a change in D7.

Neograph734’s picture

That feels a bit like a hack, however the only alternative I could come up with, is always calling the function manually which is an even worse idea...

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

I don't see how we can add a method to an interface in a stable release... for example, I did a little testing and this patch causes fatal errors with things like https://drupal.org/project/media_youtube due to the missing method (basically any module that uses the interface but doesn't happen to extend DrupalLocalStreamWrapper).

I'm not really sure what to do about this, but it needs more review. What would happen if we added the method to the implementing class(es), but didn't add it to the interface itself? Followup issues would need to be created for the Media module (and others) to add it there anyway, but nothing would break in the meantime.

Also some minor issues:

   public function stream_stat();
+  /**
...
+  public function stream_metadata($uri, $option, $value);
   public function unlink($uri);

If there's PHP doc, there should be a blank line at the top and bottom separating this method from the others.

+   * This is a new method in PHP5.4:
+   * @see http://www.php.net/manual/en/migration54.methods.php.
+   *
+   * In PHP5.3 use the Drupal specific method DrupalStreamWrapperInterface::chmod().

I think this might need a stronger warning; I didn't fully get that it meant the code we're adding to core here would be non-functional in PHP 5.3 and earlier (but it is, because the constants are undefined). That's unusual to say the least. Would it be reasonable to just indicate that this method should never be called directly (unless you are writing custom code that will never run on older versions of PHP), but that it's only there because PHP 5.4 and higher will call it internally?

Also should be "PHP 5.4" and "PHP 5.3" (with a space)... and "PHP 5.3 and earlier" since we support older versions.

   /**
+   * {@inheritdoc}
+   */

I don't think Drupal 7 does this anywhere (at least not yet).

David_Rothstein’s picture

Also, should this actually be critical? (I gathered from skimming the issue that the current situation mainly only causes PHP warnings.)

fietserwin’s picture

Priority: Critical » Normal

- We could go for implementing it in DrupalLocalStreamWrapper only. that would indeed prevent contrib to fail and would make the @inheritdoc remark disappear
- stream_metadata() should indeed never be called directly. Let's make that clearer indeed.
- The PHP warnings tell us that the executing code was not able to perform the requested operation, and thus that a chmod (or touch) simply failed so it is an error.
- I guess it is not critical for D7, as it only appears on PHP 5.4 and most code is programmed to not call chmod() with stream wrappers anyway, see e.g. drupal_chmod().

BR0kEN’s picture

I've wrote an additional interface that extends from DrupalStreamWrapperInterface and contains the definition of newest methods that needs to be implemented for each stream wrapper object for PHP 5.4 and greater. For now, the DrupalLocalStreamWrapper implements the new interface and contain implementation of necessary methods. So, modules that implements old interface will not be affected.

All code has been ported from Drupal 8.

twistor’s picture

Status: Needs review » Needs work
  1. +++ b/includes/stream_wrappers.inc
    @@ -219,6 +219,113 @@ interface DrupalStreamWrapperInterface extends StreamWrapperInterface {
    + * Interface DrupalInnovativeStreamWrapperInterface.
    + *
    

    Not sure about this name.

  2. +++ b/includes/stream_wrappers.inc
    @@ -219,6 +219,113 @@ interface DrupalStreamWrapperInterface extends StreamWrapperInterface {
    +interface DrupalInnovativeStreamWrapperInterface extends DrupalStreamWrapperInterface {
    +  /**
    +   * Sets metadata on the stream.
    

    Newline.

    Lots of these.

Blech, I would prefer if we didn't have a new interface and just implemented the missing methods. The whole idea of having an interface for stream wrappers is the wrong approach.

BR0kEN’s picture

Status: Needs work » Needs review

Newline is not needed.

Nothing wrong with the new interface. It should be to provide a possibility to implement own stream wrappers according to new PHP specification.

twistor’s picture

Status: Needs review » Needs work

Yes, newlines are needed in-between each method definition in the interface, including before the first one.
See https://www.drupal.org/node/608152.

The name DrupalInnovativeStreamWrapperInterface is meaningless.

See #2501707: StreamWrapperInterface doesn't have to implement PhpStreamWrapperInterface as to why interfaces for stream wrappers are an incorrect approach.

There is nothing stopping anyone from implementing the new methods without an interface.

BR0kEN’s picture

Status: Needs work » Needs review
FileSize
5.34 KB
8.8 KB

I know that Drupal CS means that newline is needed, but this file was written in a one style and I will not change it in one place.

About an interface - is a moot point. On the one hand, it is not needed for implementing objects provided by language, but, on another - needed for preventing some errors from developers. So, I don't want to argue and just made an additional patch.

BR0kEN’s picture

Status: Needs review » Reviewed & tested by the community

Maybe one of responsible persons give some answer?

Darren Oh’s picture

This patch fixes the issue and has only one style error. The comment on the stream_set_option method has two /** lines.

BR0kEN’s picture

BR0kEN’s picture

BR0kEN’s picture

Fabianx’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work
Issue tags: +Modern PHP
Related issues: +#2656548: Fully support PHP 7.0 in Drupal 7
+++ b/includes/stream_wrappers.inc
@@ -550,6 +549,144 @@ abstract class DrupalLocalStreamWrapper implements DrupalStreamWrapperInterface
+   * This is a new method in PHP5.4:
+   * @link http://www.php.net/manual/en/migration54.methods.php.
+   *
+   * In PHP5.3 use the Drupal specific method

Lets change the wording to say:

This is a method that is supposed to be only called internally by PHP itself.

Do not call this directly.

If you still want to support PHP 5.3 use DrupalStreamWrapperInterface::chmod.

--

Or something like that.

Docs need to be way clearer. Also see David's remarks.

BR0kEN’s picture

Status: Needs work » Needs review
FileSize
5.38 KB
1.4 KB

Guys, please note that this patch - is a backport from Drupal 8.

BR0kEN’s picture

So?

Fabianx’s picture

#72: I still think we should have a warning to not use it if you want to support <= PHP 5.3.

Drupal 8 does not care about that, but we do.

The patch itself looks good. But please add the discussed doc lines.

BR0kEN’s picture

Make sense. Also I've added obvious definition of visibility for DrupalStreamWrapperInterface::setUri() method.

Fabianx’s picture

+++ b/includes/stream_wrappers.inc
@@ -550,6 +549,152 @@ abstract class DrupalLocalStreamWrapper implements DrupalStreamWrapperInterface
+   * WARNING: This method is called internally by PHP itself. Do not
+   * call it directly. If you still want to support PHP 5.3 and lower:
+   * @see DrupalStreamWrapperInterface::chmod().

The warning could be a little clearer still.

I am not yet sure what you would call when:

e.g. In PHP 5.5 would you call this directly or also use chmod?

Thanks!

BR0kEN’s picture

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Drupal bugfix target

RTBC, this looks great to me now and has parity with D8.

  • catch committed 35a33d6 on 8.3.x
    Issue #2107287 by sun, amateescu: PHP 5.4 calls a new stream_metadata()...

  • catch committed 35a33d6 on 8.3.x
    Issue #2107287 by sun, amateescu: PHP 5.4 calls a new stream_metadata()...
BR0kEN’s picture

fago’s picture

Status: Reviewed & tested by the community » Needs work

I ran into warnings "Drupal\Core\StreamWrapper\PublicStream::stream_metadata is not implemented!" when using some composer package with Drupal 7 - thus I found this issue.

I reviewed the patch in #76 and it looks mostly good to me as well. Also, I tested the patch and verified that the warnings are successfully solved.

However, I found those two docblock remarks/nitpicks, so setting to "needs-work" for those:

  1. +++ b/includes/stream_wrappers.inc
    @@ -550,6 +549,155 @@ abstract class DrupalLocalStreamWrapper implements DrupalStreamWrapperInterface
    +   * @see touch()
    

    According to https://www.drupal.org/node/1354#order @see should come much later. If you want to mention functions as part of the text, just do so without using @see. @see can add some general referenced next to @link though.

  2. +++ b/includes/stream_wrappers.inc
    @@ -550,6 +549,155 @@ abstract class DrupalLocalStreamWrapper implements DrupalStreamWrapperInterface
    +   * @see http://php.net/manual/streamwrapper.stream-cast.php
    

    Should be @link.

BR0kEN’s picture

And again I've got back to this issue...

Status: Needs review » Needs work

The last submitted patch, 82: drupal-stream-wrapper-php54-2107287-82.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC

BR0kEN’s picture

Please commit somebody!!

  • catch committed 35a33d6 on 8.4.x
    Issue #2107287 by sun, amateescu: PHP 5.4 calls a new stream_metadata()...

  • catch committed 35a33d6 on 8.4.x
    Issue #2107287 by sun, amateescu: PHP 5.4 calls a new stream_metadata()...
stefan.r’s picture

Issue tags: +Pending Drupal 7 commit

  • stefan.r committed 198ba65 on 7.x
    Issue #2107287 by BR0kEN: PHP 5.4 calls a new stream_metadata() method...
stefan.r’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Committed and pushed to 7.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.