Re: [RFC][Discussion] Add support for stream-wrapped URLs in opcode cache

Hi Dan, Thanks for your comments. Le 03/06/2017 à 15:34, Dan Ackroyd a écrit :

However as you declined to respond on Github, I'll ask here again:

I am sorry to say that I didn't decline anything, as you never commented the PR. It seems I didn't reply to questions you didn't ask :). I also prefer technical discussions to occur on github.

1) Why is the patch checking for PHP's own version? This doesn't occur anywhere else in the code

The PR had no target version and I wanted to show how to make an extension compatible with older versions. But I agree there's no need for such compatibility among core components. So, I removed the '#if" checks and just added a small comment in php_streams.h to make extension developers' life easier.

2) Why does the patch modify PHP_API_VERSION, which was already modified for PHP 7.2

Right. As the patch had no target version, I had defined an arbitrary value to check. This is now reverted to the value defined for 7.2.

Additionally, the section for 'Backward Incompatible Changes' has 'None' listed - but then it lists some required modifications in "To Existing Extensions" - won't non-core PHP extensions also be affected by this RFC?

Good question. The 'phar' and 'plain' wrappers had to be modified because their names were hardcoded in the opcache code. Every other stream wrapper (core or not) is currently considered as 'non-cacheable' and will remain, as long as it does not implement a 'cache_key' operation. So, we can be sure that no other extension will be impacted by the RFC. Actually, any 3rd-party opcode cache would have to be modified to become able to cache stream-wrapped URLs (which would not be a BC break). But, AFAIK, opcache is the only opcode cache supported by PHP 7. So, I don't think I need to list this in the RFC. Cheers François