Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:114870
Message-ID: <CB75D867-6BA1-41A2-AF5C-7BE2D89B1FB1@newclarity.net>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_B186D133-6AD6-4E66-A8E5-7225AE846266"
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Date: Mon, 14 Jun 2021 15:16:27 -0400
In-Reply-To: <CAFv4g+EhAd-b7eRkKxAWK-bRWuWhciD5wzr2ArSp0kU6S4gkwg@mail.gmail.com>
Cc: PHP internals <internals@lists.php.net>
To: Craig Francis <craig@craigfrancis.co.uk>
References: <CAFv4g+F-7F7o6va5wCrojKe+Uysx2Ec3H4yV_mUZYEyNm3MkdQ@mail.gmail.com>
 <D1B12C84-A0FE-4D3E-811C-DF38F1FA4469@newclarity.net>
 <CAFv4g+ES=592UbazDbkZB7AB485hu9gh+=qG-A+KWgOWD4ArVw@mail.gmail.com>
 <CFB0B0B2-A646-4DB1-83B9-DEF7907EDD7E@newclarity.net>
 <5630B850-23F4-4734-B916-D9E181B491AC@newclarity.net>
 <429945ed-c328-e3b0-796f-62d7bc1bfe18@processus.org>
 <CAFv4g+EhAd-b7eRkKxAWK-bRWuWhciD5wzr2ArSp0kU6S4gkwg@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] is_literal
From: mike@newclarity.net (Mike Schinkel)

--Apple-Mail=_B186D133-6AD6-4E66-A8E5-7225AE846266
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

Hi Craig,

> On Jun 14, 2021, at 5:36 AM, Craig Francis <craig@craigfrancis.co.uk> =
wrote:
>=20
> On Mon, 14 Jun 2021 at 09:07, Pierre <pierre-php@processus.org> wrote:
>=20
>> Le 14/06/2021 =C3=A0 02:41, Mike Schinkel a =C3=A9crit :
>>> A big*NO*  on warnings. Full stop.
>>=20
>> [...] Any warning raised by the low level functions would be too
>> restrictive.
>=20
> Hi Pierre, I'll be talking to Mike on Zoom later today (5pm UK time), =
which
> you're welcome to join.

Thank you for making yourself available to discuss the RFC on a Zoom =
chat.  It was super helpful to be able to hash out concerns without =
getting into an avalanche of email, and frankly it would be great if all =
future RFC authors were to do the same. I was able to better understand =
your concerns, and hopefully you were better able to understand mine.

Just to follow up, I wanted to re-iterate that I think it is critical =
that you give users in userland an "escape valve" for when library =
authors =E2=80=94 including junior developers  =E2=80=94 force users to =
provide a literal but the use-case makes that impossible.

I think we both agreed that the most promising thing we discussed would =
be if userland could create a "trusted object" using some combination of =
implementing an interface and/or using an attribute such as you see in =
the example below:

class PostGetter implements LiteralInterface {
    private string $post_id;
    public function __construct( int $post_id ) {
        $this->post_id =3D intval($post_id);
        if ( 0 =3D=3D=3D  $this->post_id ) {
            throw new Exception( "Sorry, your post_id is not a valid =
non-zero integer." );
        }
    }
    #[IsLiteral]
    public function __ToString() {
        return sprintf( "SELECT * FROM wp_post WHERE ID=3D%d", =
$this->post_id );
    }
}

// $sql accepts a literal string, or a LiteralInterface implementor=20
// having a __ToString() method with the #[IsLiteral] attribute
function safe_sql_query( string|LiteralInterface $sql =
):mysqli_result|false {
    if ( ! is_literal( $sql ) ) {
        throw new Exception( "Sorry, you cannot use an non-literal =
here." );
    }
    $connection =3D mysqli_connect( ... );
    return mysqli_query( $connection, $sql );
}

//
// Our LiteralInterface implementor in use
//
$pg  =3D new PostGetter( $_GET['post_id'] );
$result =3D safe_sql_query( $pg );
print_r( $result );

You said you wanted to think about this approach so I am looking forward =
to yours and other's thoughts on the validity of this approach.=20

Note that I mentioned that some people might have an issue with using =
the __ToString() magic method and instead might want named methods =
defined in an interface to be used instead.  So maybe LiteralInterface =
could define a method asLiteral() that the class would need to =
implement.  And in a future RFC maybe SqlLiteralInterface could define =
asSqlLiteral(), etc.  These are the obviously points to be hashed out on =
the list.=20

Also note I used sprintf() in my example rather than mysqli_prepare() on =
purpose because WordPress uses mysqli_query() and mysqli_query() expects =
SQL as a string, not a prepared statement. I could have used =
$wpdb->prepare() but then many people on the list might not be familiar =
with that aspect of WordPress and safe_sql_query() is an approximation =
of $wpdb->get_results() assuming $wpdb were an instance of a newer =
object that checked is_literal().

As a final comment I know you mentioned that we only have two weeks to =
get this RFC into the next release of PHP and that is why you are =
anxious to avoid addressing how userland can create "trusted objects" =
for this RFC  However, I think it would be far better to address that =
concern in this RFC even if that means delaying is_literal() until a =
later version PHP and instead provide time to addresses this concern and =
any other's concerns.

-Mike

P.S. Also, happy to do another Zoom call if you would like to hash any =
of this out further, just email me privately.



--Apple-Mail=_B186D133-6AD6-4E66-A8E5-7225AE846266--