Add is_cacheable() stream wrapper operation #976
Conversation
|
Big 👍 |
| static int phar_wrapper_is_cacheable(php_stream_wrapper *wrapper, const char *url, int options, php_stream_context *context) /* {{{ */ | ||
| { | ||
| /* Phar URLs are always cacheable */ | ||
| return 1; |
| @@ -44,7 +44,11 @@ php_stream_wrapper_ops phar_stream_wops = { | |||
| phar_wrapper_unlink, /* unlink */ | |||
| phar_wrapper_rename, /* rename */ | |||
| phar_wrapper_mkdir, /* create directory */ | |||
| phar_wrapper_rmdir, /* remove directory */ | |||
| phar_wrapper_rmdir /* remove directory */ | |||
| #ifdef PHP_STREAMS_SUPPORT_IS_CACHEABLE | |||
There was a problem hiding this comment.
I don't see why you need this ifdef if it's always going to be available... now you also have to mess up the codeing standards (, goes at the end).
There was a problem hiding this comment.
I did it this way because I imagined that the phar code had to be kept compatible with previous PHP versions (but maybe the phar reference is not here, or development for PHP7 has been split).
If we consider that this code is synchronized with is_cacheable() support, yes, we can drop the conditionals.
There was a problem hiding this comment.
I agree, having this define looks pointless - if we accept this RFC, this is part of PHP 7 and should be checked against as "is this PHP 7?" if somebody needs to maintain cross-version extensions. We don't have separate defines for every PHP 7 feature, and it's not configurable in any way, so it should be just there - or, if version portability is needed (not sure for phar, it's a core module, why would it need that?) use PHP version checks.
There was a problem hiding this comment.
Thanks for your comments. Now that it is clear that the feature is for PHP7, I’ll remove the define and enclose phar changes in a PHP_VERSION-based ifdef.
50c7a8b to
195d9d0
Compare
|
|
||
| if (call_result == SUCCESS) { | ||
| /* We got the info we needed */ | ||
| convert_to_long(&zretval); |
There was a problem hiding this comment.
Why long? You just documented it as being true/false. Then you should use zend_is_true or something like that.
058403d to
920a85f
Compare
|
@flaupretre Any news? This would be very useful. |
|
@marcioAlmada Sure. The feature missed the 7.0 deadline, but is planned for 7.1. An additional reason to implement it is that it is required by PCS, which I'll also propose for inclusion in 7.1. Of course, both features will go through the RFC process first and the result is unpredictable ;) but the first returns I got are positive. Instead of is_cacheable(), I will propose a cache_key() operation which will return NULL if the URL is not cacheable, and a zend_string to use as a key if the URL can be cached. |
920a85f to
cce702a
Compare
|
Follow-up: #1711 |
Please refer to the corresponding RFC :
https://wiki.php.net/rfc/streams-is-cacheable