Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:99350
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: unknown (pb1.pair.com: domain php.net does not designate 212.27.42.2 as permitted sender)
To: Dan Ackroyd <danack@basereality.com>,
 =?UTF-8?Q?Fran=c3=a7ois_Laupretre?= <francois@tekwire.net>
Cc: Internals <internals@lists.php.net>
References: <1e99c642-e3b8-3445-82ac-f6cb6258e0b4@php.net>
 <CA+kxMuTseKk4S5WwBd-cHDAPe6EOs-8rNyksG0Sje=XLYBPU_Q@mail.gmail.com>
 <e1567402-5e2c-d7ee-583c-30e40a818e70@tekwire.net>
 <CA+kxMuRuSwSviZ9bNZq9Que7iuPCd6V7hLuR1LMb-hSmnHesYw@mail.gmail.com>
Message-ID: <0a47f5a7-8f32-9f6e-b4da-81933dbb85dc@php.net>
Date: Sun, 4 Jun 2017 12:03:49 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.1.1
MIME-Version: 1.0
In-Reply-To: <CA+kxMuRuSwSviZ9bNZq9Que7iuPCd6V7hLuR1LMb-hSmnHesYw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [RFC][Discussion] Add support for stream-wrapped URLs
 in opcode cache
From: francois@php.net (=?UTF-8?Q?Fran=c3=a7ois_Laupretre?=)

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