[TYPO3-core] RFC #15457: Add support for prepared queries [summary and pending questions]

[Jigal van Hemert]

Hi,

Thanks for all your thinking, answers and unlimited patience :-)

On 19-8-2010 9:13, Xavier Perseguers wrote:
>  > PARAM_AUTOTYPE is not present in PDO, so this will require extra
>  > code when using PDO itself
>
> After a few tests, Using type PARAM_STR instead seems fine and works (by
> testing) with numerical columns such as pid or uid. And it behaves like
> PDO.
> Same problem happens with PARAM_FLOAT which does not exist in PDO and
> could be handled as PARAM_STR. PARAM_INT should stay as it is (now with
> latest patch), that is, an integer!

If the default PARAM_STR works correctly. PARAM_AUTOTYPE isn't really 
necessary, if bindValues() uses it's behaviour internally (see below).

>  > Do not default PARAM_STR
> I do not agree. Either we keep PARAM_AUTOTYPE and as such do not default
> to PARAM_STR or behave like PDO but forcing the user to provide a
> datatype means:
> 1) bindValues() and execute($params) will be completely unusable as user
> should then provide an array within an array for parameters

This could be solved by using PARAM_AUTOTYPE's behaviour internally.

> 2) Would not behave like PDO and then is not in line with your wish to
> have an API as close as possible to PDO ;-)

My concern is mainly to introduce behaviour which is handled differently 
by PDO and thus blocking the use of actual PDO calls in this class later 
on (see issue of named markers only replaced once).

In this case forcing a type would not prevent us from passing that on to 
the PDO call.

Because you tested the behaviour of PARAM_STR as default and if we are 
going to use the behaviour of PARAM_AUTOTYPE internally in bindValues() 
and execute() there is nothing anymore against keeping it as default 
like PDO does.

>  > Remove method bindValues()
> I do not see the downside of having it if we default to PARAM_STR
> instead of the would-not-exist-anymore PARAM_AUTOTYPE. This way, it
> would behave as when providing parameters in method execute(). But I

I also like Ernesto's idea of letting bindValues() and execute() use the 
behaviour of PARAM_AUTOTYPE internally. Of course this needs to be 
documented. My only "if" here is that PARAM_INT is only used for 
integers and not for anything that passes the is_numeric() test.

>  > Named markers are only replaced once in PDO.
>
> The remarks is to allow or not such query:
> where :id is replaced twice. In PDO this is not the case. Still, I find
> it handy and as such would like to keep it.

It is handy, but the reason for me to say no is that it would break code 
which relies on multiple replacements when we implement PDO calls in 
this class. If we need to support multiple replacements we cannot let 
the DBMS take care of the prepared statements and we will thus lose the 
speed advantages of prepared statements.
The idea is still to let the DBMS do the syntax check, optimization and 
execution path planning once and execute the result multiple times with 
different values at certain places.

Do you see a way to do this while still allowing markers to be used more 
than once?

>  > fetch* functions in PDO default to FETCH_BOTH; is there a reason to
>  > do something else?
>
> Have you real use-cases where you rely on having both indexed- and
> named-based keys in your resultset? How is your code using
> $GLOBALS['TYPO3_DB'] currently written to work like that?

So the answer is "because t3lib_db defaults to FETCH_ASSOC". Fine with 
me :-)

>  > In PDO this is not possible to support both named and positionned
>  > markers as it results in exception SQLSTATE[HY093] being thrown.
> => We have no way in native MySQL to detect that both types are used in
> query. What we could do is see which type is used with first call to
> bindValue() and stick to that type for all subsequent calls...

Good solution! At least it prevents code relying on mixed markers from 
breaking when PDO calls are introduced later.

> Regarding your ancient "PDO" class. Did not test but we could use
> something like you did:

It was not for testing or to show how I think it should be done, but it 
might give you some ideas. It was an internal module, so it was very 
easy to prevent the use of mixed marker types: document that it was 
forbidden even though the code allowed it :-)

> if (!is_numeric($key)) {
> // replace the marker (not preceeded by a word character or a ':' but
> followed
> // by a word boundary)
> $this->sql = preg_replace('/(?<![\w:])'.$key.'\b/', PDO::quote($val,
> $data_type), $this->sql);
> }
> instead of str_replace() when using native MySQL (when using DBAL we
> have a proper way of replacing both question marks and named markers).

I think that str_replace is a bit faster. So, if that works fine let's 
keep it.

> Regarding constant values. It does not cost anything to use the real
> internal value of PDO instead of sequential value. As such I'd like to
> have it as in PDO (PARAM_INT, PARAM_BOOL, PARAM_STR).

I don't see a problem with constant values. At the time I wrote that 
class I may not have known the internal values, so the constants may 
have had imaginary values.
Your constants look fine to me.

> Possible support for INSERT, UPDATE, ... may be added in future but will
> definitively not with this RFC.

Agree.

I'm trying to test the patch later today.

Thanks for your great work on the database part!

-- 
Kind regards / met vriendelijke groet,

Jigal van Hemert
skype:jigal.van.hemert
msn: jigal at xs4all.nl
http://twitter.com/jigalvh