Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114868
To: internals@lists.php.net
References: <CAFv4g+F-7F7o6va5wCrojKe+Uysx2Ec3H4yV_mUZYEyNm3MkdQ@mail.gmail.com>
 <CAOwTYAt3rztsZ1BTjEbsonqPR_thgG7UqEUO2pKeN=dEg1jqgg@mail.gmail.com>
 <CA+kxMuSJvk2bDM1dQnZHxgiO+YrYhbjS8BziAy+YGSSW8cR1nw@mail.gmail.com>
Message-ID: <eff0cd38-1084-d4e9-a2ab-4e3eedd461c6@gmail.com>
Date: Mon, 14 Jun 2021 19:43:52 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <CA+kxMuSJvk2bDM1dQnZHxgiO+YrYhbjS8BziAy+YGSSW8cR1nw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-GB
Subject: Re: [PHP-DEV] [RFC] is_literal
From: rowan.collins@gmail.com (Rowan Tommins)

Hi Dan,

On 14/06/2021 13:29, Dan Ackroyd wrote:

> While I think the idea behind this RFC is great, the current
> implementation is terrible, as it will cause some fatal errors in
> production.


I think you have raised a legitimate concern here, but agree with 
Matthew Brown that calling the implementation "terrible" is neither 
warranted nor helpful.

It's also a bit bizarre to criticise the RFC for causing fatal errors 
even though it doesn't introduce any, then propose a different version 
which *does* introduce fatal errors.


> The only information would be that the html string is not
> literal....but there wouldn't be any info about which bit was the
> problem. Someone would have to manually check which code had been
> called to produce the non-literal string.


The difficulty of tracing why something is not in the expected state is 
indeed a tricky one, but I don't think mandating literal_implode solves 
it well enough to be worth the pain. Essentially, changing getInfoPanel 
to use literal_implode is equivalent to this:



> function getInfoPanel(UserPreferences $up): string
> {
>      $html = "<div style='color: " . $up->getColor() . "'>";
>      $html .= // more html here
>      $html .= "</div>"
>
>      assert( is_literal($html) );
>      return $html;
> }


However, there could still be an arbitrarily long code path between the 
user input and that assertion (or call to literal_implode), so tracking 
the root cause could be just as difficult.

The actual root cause is in getColor(), so the actual assertion needed 
is there:


> class UserPreferences {
>      private DB $db;
>
>      function getColor(): string {
>          $preferredColor = $this->db->getColor();
>
>          if ($preferredColor === 'light') {
>              return '#fff';
>          }
>
>          if ($preferredColor === 'dark') {
>              return '#000';
>          }
>          assert( is_literal($preferredColor) );
>          return $preferredColor; // user has set custom color.
>      }
> }

Adding that assertion would be easier and more elegant if it was 
possible to set a "literal_string" return type, but it would still be up 
to the user to add it. That leads to the original problem, but in 
reverse: when writing the getColor() function, the author needs to know 
where it will eventually be used to know that an assertion is relevant.

It would also not solve the problem of the assertion only triggering in 
production without a careful integration test. That can only be solved 
with a static analyser, which would also have enough information to 
isolate the cause.

Regards,

-- 
Rowan Tommins
[IMSoP]