Everything

php

pdo: patch to make leading colon unneeded for bound variables without breaking BC.

php.internals

Thies C . Arntzen

21 years ago

subject says it -
testcase is also attached.

i have updated: sqlite and oci.
i have tested: sqlite and postgres (uses bind emulation).

i did not look at the firebird code as it seemed "complexish" to me;-)

all the other drivers seem to use the emulation (like postgres) so they
should work.

re, thies

Unnamed Person

21 years ago

"Thies C. Arntzen" <thies@thieso.net> writes:

> diff -u -w -r1.95 pdo_stmt.c
> --- ext/pdo/pdo_stmt.c	24 Mar 2005 12:32:06 -0000	1.95
> +++ ext/pdo/pdo_stmt.c	25 Mar 2005 13:13:18 -0000
> @@ -242,6 +242,13 @@
>
>  	hash = is_param ? stmt->bound_params : stmt->bound_columns;
>
> +	if (param->name) {
> +		if (param->name[0] == ':') {
> +			param->name = param->name + 1;

Sanity check...

Without researching thoroughly, this "appears" to warrant further inspection.
The field param->name is clearly a pointer since you're able to increment it.
It seems likely, then, that param->name was dynamically allocated.  If that's
not the case, then you can stop reading.  If it is, though, then by
incrementing param->name, what happens when param->name is freed?  I didn't
see any place in the patch where it is marked as having been incremented nor
anyplace where it is decremented in order to be freed.

Cheers,

Derrell

Thies C . Arntzen

21 years ago

Am 25.03.2005 um 15:28 schrieb Derrell.Lipman@UnwiredUniverse.com:

> "Thies C. Arntzen" <thies@thieso.net> writes:
>
>> diff -u -w -r1.95 pdo_stmt.c
>> --- ext/pdo/pdo_stmt.c	24 Mar 2005 12:32:06 -0000	1.95
>> +++ ext/pdo/pdo_stmt.c	25 Mar 2005 13:13:18 -0000
>> @@ -242,6 +242,13 @@
>>
>>  	hash = is_param ? stmt->bound_params : stmt->bound_columns;
>>
>> +	if (param->name) {
>> +		if (param->name[0] == ':') {
>> +			param->name = param->name + 1;
>
> Sanity check...
>
> Without researching thoroughly, this "appears" to warrant further
> inspection.
> The field param->name is clearly a pointer since you're able to
> increment it.
> It seems likely, then, that param->name was dynamically allocated.  If
> that's
> not the case, then you can stop reading.  If it is, though, then by
> incrementing param->name, what happens when param->name is freed?  I
> didn't
> see any place in the patch where it is marked as having been
> incremented nor
> anyplace where it is decremented in order to be freed.
>
>

acutally i did  a bit of checking...

really_register_bound_param is called from two places... and at the end
it does:

if (param->name) {
param->name = estrndup(param->name, param->namelen);

so i believe my patch is safe.

i agree that it could|should be done nicer...

-tc

Jani Taskinen

21 years ago

Excuse me, but what BC? I don't think this stuff has been
released yet, so how could you break BC? :)

--Jani

On Fri, 25 Mar 2005, Thies C. Arntzen wrote:

Wez Furlong

21 years ago

There are releases on pecl.php.net, and there are (a few) people
running these in production. In addition, it's been advertised as
working this way for over a year.

So, it is important to preserve the current behaviour when this patch
is applied (will do so this weekend; need to catch up on a lot of
things first).

--Wez.


On Sat, 26 Mar 2005 01:39:59 +0200 (EET), Jani Taskinen <sniper@iki.fi> wrote:

Edin Kadribasic

21 years ago

On Mar 26, 2005, at 1:47, Wez Furlong wrote:

> There are releases on pecl.php.net, and there are (a few) people
> running these in production. In addition, it's been advertised as
> working this way for over a year.
>
> So, it is important to preserve the current behaviour when this patch
> is applied (will do so this weekend; need to catch up on a lot of
> things first).

Hehe, this is too funny. Last time you dismissed my BC concerns when I
said we should remove ":" now by saying something along the lines, we
can always make things less strict. If this does not make sense now its
because it never did :)

Edin

Wez Furlong

21 years ago

On Sat, 26 Mar 2005 02:13:54 +0100, Edin Kadribasic <edink@emini.dk> wrote:

>
> On Mar 26, 2005, at 1:47, Wez Furlong wrote:
>
> > There are releases on pecl.php.net, and there are (a few) people
> > running these in production. In addition, it's been advertised as
> > working this way for over a year.
> >
> > So, it is important to preserve the current behaviour when this patch
> > is applied (will do so this weekend; need to catch up on a lot of
> > things first).
>
> Hehe, this is too funny. Last time you dismissed my BC concerns when I
> said we should remove ":" now by saying something along the lines, we
> can always make things less strict. If this does not make sense now its
> because it never did :)

I've had code running using ':' since before this topic was first raised.
I don't see what's funny about it; the fact is that there is code
alive using : prefixes; we can't simply stop supporting it.

Thies' patch is simply "making things less strict", and that is not
something I had ruled out, just a decision I wanted to defer.

--Wez.

Lester Caine

21 years ago

Wez Furlong wrote:

>>>There are releases on pecl.php.net, and there are (a few) people
>>>running these in production. In addition, it's been advertised as
>>>working this way for over a year.
>>>
>>>So, it is important to preserve the current behaviour when this patch
>>>is applied (will do so this weekend; need to catch up on a lot of
>>>things first).
>>
>>Hehe, this is too funny. Last time you dismissed my BC concerns when I
>>said we should remove ":" now by saying something along the lines, we
>>can always make things less strict. If this does not make sense now its
>>because it never did :)
>
> I've had code running using ':' since before this topic was first raised.
> I don't see what's funny about it; the fact is that there is code
> alive using : prefixes; we can't simply stop supporting it.

So the way things have been 'standardised' currently are cast in stone,
even if maintaining them introduces complex checks across other drivers?

> Thies' patch is simply "making things less strict", and that is not
> something I had ruled out, just a decision I wanted to defer.

I would have thought the reference should be the SQL standard rather
than "making things less strict" - Having to ADD the ':' is something I
don't remember having to do for some time - in production.

Since available time is short, playing with pdo is still being deferred
- at least until I can get pdo_firebird to do anything useful ;)

--
Lester Caine
-----------------------------
L.S.Caine Electronic Services

Thies C . Arntzen

21 years ago

Am 26.03.2005 um 01:47 schrieb Wez Furlong:

> There are releases on pecl.php.net, and there are (a few) people
> running these in production. In addition, it's been advertised as
> working this way for over a year.
>
> So, it is important to preserve the current behaviour when this patch
> is applied (will do so this weekend; need to catch up on a lot of
> things first).

actually this patch needs some work (as it's a 20 minute hack on code
i've never seen before).

i seem to be really unlucky in getting my intention across. by accident
i started a thread about: a) bad coding style /and/ b) BC breakage when
all i ever wanted was c)  to make the colons for bound variables
optional...

guys, can we please get back to the subject? this list reminds me of
driving a _too_ old and broken car: "one hour of driving, 2 hours of
fixing" - if it was "one hour of coding, 4 hours of
documenting/testing/qa/discussing the right way.." it would be fine by
me but right now it seems more like "1 hour of work, 3 hours of
comparing dick-sizes with the SmarterGuys(tm)". that is screwed up!

thies