Problem
- PHP 5.4 calls a new
stream_metadata()
method on stream wrappers forchmod()
,touch()
,chown()
, andchgrp()
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
- Drop the custom
StreamWrapperInterface::chmod()
method and require all stream wrappers to implementstream_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.
Comment | File | Size | Author |
---|---|---|---|
#82 | interdiff-2107287-76-82.txt | 1.13 KB | BR0kEN |
#82 | drupal-stream-wrapper-php54-2107287-82.patch | 5.91 KB | BR0kEN |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedI'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.
Comment #2
fietserwinThanks for joining this issue.
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.
Comment #3
amateescu CreditAttribution: amateescu commentedThat'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.
Comment #4
fietserwinAfter 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:
and adapted the methods chmod and stream_metadata of Localstream to also do some logging:
To my surprise (and yours as well I guess) this is what I found in the log file:
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.
Comment #5
amateescu CreditAttribution: amateescu commentedI'm sorry but I don't see what the bug is :)
Comment #6
fietserwinI 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().
Comment #7
amateescu CreditAttribution: amateescu commentedHm.. ok, that's weird. Unfortunately, I'm not so versed in stream wrappers so I can't say how to proceed :/
Comment #7.0
amateescu CreditAttribution: amateescu commentedUpdated issue summary.
Comment #8
sunLet'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.
Comment #9
sunOk, 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.......
Comment #11
sunClarifying 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?
Comment #12
fietserwinLet'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.
Comment #13
fietserwin9: drupal8.stream-metadata.9.patch queued for re-testing.
Comment #15
fietserwinNow 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.
Comment #16
fietserwinI 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.
Comment #17
fietserwinCreated follow-up issue #2213241: Fully conform to PHP5.4 streamwrapper class.
Comment #18
fietserwinChild issues do not have to be listed as a related issue...
Comment #19
fietserwinAccording to https://groups.drupal.org/node/413508, the test bot is on 5.4.
Comment #20
fietserwin9: drupal8.stream-metadata.9.patch queued for re-testing.
Comment #22
sunFixed 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...)
Comment #24
fietserwinI'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;
Results of the above code:
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.
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.No, it acts on a stream, and on a stream only.
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.
No, it should not. Reading on a closed stream wrapper should do the same thing as reading on a closed file handle.
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.
Comment #26
fietserwinOK, 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:
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?
Comment #27
sunAdded back clearstatcache().
I omitted that, because I did not understand why we'd have to explicitly clear it when the
touch()
andchmod()
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?
Comment #28
sunHm. 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?Comment #29
fietserwinIf 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:
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.
Comment #30
sunAlright, 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.
Comment #31
sunLet's move forward here?
Comment #32
fietserwin2 small points, though IMO neither one is blocking, so I would be fine with RTBC'ing this one. Someone else?
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.
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.
Comment #33
sunPHP 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
I'd like to limit the scope of this issue to add the missing
stream_metadata()
method.The current
LocalStream
wrapper in HEAD performs theclearstatcache()
after achmod()
operation. While I basically agree (as mentioned in previous comments), removing it is a change in expected behavior, which should be discussed separately.Comment #34
twistor CreditAttribution: twistor commentedUgh. What a sad API. This looks correct.
Comment #35
fietserwin+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.
Comment #36
catchOK 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.
Comment #38
sunI'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.
Comment #39
Neograph734Oke, 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.
Comment #41
Neograph734Forgot 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.
Comment #43
fietserwinThere are hard tabs in your patch, please use spaces according to the coding standards.
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.
Comment #44
Neograph734I 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.
Comment #45
Neograph734This looked good on local.
Comment #46
Neograph734Comment #47
Neograph734It 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.
Comment #48
fietserwinre #47:
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.
Comment #49
Neograph734Ok, 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.
Comment #50
fietserwinI am OK with the new patch, though one minor documentation detail:
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().
Comment #51
Neograph734Here you go
Comment #52
Neograph734Comment #53
fietserwinRTBC, 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.
Comment #54
Neograph734I 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.
Comment #55
fietserwinI 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.
Comment #56
Neograph734That 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...
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commentedI 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:
If there's PHP doc, there should be a blank line at the top and bottom separating this method from the others.
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.
I don't think Drupal 7 does this anywhere (at least not yet).
Comment #58
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, should this actually be critical? (I gathered from skimming the issue that the current situation mainly only causes PHP warnings.)
Comment #59
fietserwin- 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().
Comment #60
BR0kENI'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, theDrupalLocalStreamWrapper
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.
Comment #61
twistor CreditAttribution: twistor commentedNot sure about this name.
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.
Comment #62
BR0kENNewline 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.
Comment #63
twistor CreditAttribution: twistor commentedYes, 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.
Comment #64
BR0kENI 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.
Comment #65
BR0kENMaybe one of responsible persons give some answer?
Comment #66
Darren OhThis patch fixes the issue and has only one style error. The comment on the stream_set_option method has two /** lines.
Comment #67
BR0kENOh, thanks, @Darren Oh. I've not seen this.
Comment #68
BR0kENComment #69
BR0kENComment #70
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedLets 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.
Comment #71
BR0kENGuys, please note that this patch - is a backport from Drupal 8.
Comment #72
BR0kENSo?
Comment #73
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#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.
Comment #74
BR0kENMake sense. Also I've added obvious definition of visibility for
DrupalStreamWrapperInterface::setUri()
method.Comment #75
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThe 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!
Comment #76
BR0kENComment #77
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC, this looks great to me now and has parity with D8.
Comment #80
BR0kENComment #81
fagoI 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:
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.
Should be @link.
Comment #82
BR0kENAnd again I've got back to this issue...
Comment #84
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedBack to RTBC
Comment #85
BR0kENPlease commit somebody!!
Comment #88
stefan.r CreditAttribution: stefan.r commentedComment #90
stefan.r CreditAttribution: stefan.r commentedCommitted and pushed to 7.x, thanks!