1.6.2 DB bug - empty strings not recognized in AR

[eluser]GDias[/eluser]
Hello again,

Let's look at CI's 1.6.1 code.

The old code:

Code:

if ( ! is_null($v))
{
    if ($escape === TRUE)
    {
        // blah
    }
    if ( ! $this->_has_operator($k))
    {
        $k .= ' =';
    }
    
    $v = ' '.$this->escape($v);
}
else
{
    $k = $this->_protect_identifiers($k, TRUE);
}

The new code changed the line

Code:

$v = ' '.$this->escape($v);

for:

Code:

if ($v !== '' AND $v !== NULL)
{        
    if ($escape === TRUE)
    {
        $v = ' '.$this->escape($v);
    }
}

but note that line above all:

Code:

if ( ! is_null($v))

So any comparison of $v with NULL makes no sense at all in this part of code.

The code that run when $v is NULL also changed from:

Code:

$k = $this->_protect_identifiers($k, TRUE);

for:

Code:

if ($escape === TRUE)
{
    $k = $this->_protect_identifiers($k, TRUE);
}

So, the old code used to escape everything, but the new code only escapes NULL if $escape === TRUE and do not escape '' at all.

My proposal is not to use 1.6.2 or get only the changes you need, not the whole package.

That release with grandma's note, the view('fake') and a $v !== NULL inside of an if ( ! is_null($v)) is a big no no to me.

GD

[eluser]James Spibey[/eluser]
Sorry if this has already been stated but I didn't see it

Code:

field IS NULL

is not the same as

Code:

field != ''

is it?

If you have a non-null varchar field, to check if that field is empty you can't use IS NULL can you? They are totally different checks.

This is a regressive change and it's broken my code so I'd like to know what the official fix is?

[eluser]Armchair Samurai[/eluser]
[quote author="spib" date="1211816936"]

Code:

field IS NULL

is not the same as

Code:

field != ''

is it?[/quote]
You're absolutely correct in that in empty string does not have the same value as NULL.

(Either) Derek - has there been any discussion about this on the backend? This change appears to have FUBARed a lot of code for many users. I'm sticking with 1.6.1 until this is resolved either way, but I'm hoping for a return to the previous behavior, for reasons stately earlier by both stanleyxu and XtraFile plus I really don't feel like slogging though old code to rectify this behavior or tampering with the core.

[eluser]Derek Jones[/eluser]
Yes, we've discussed and have a change planned for the next release. I agree with Derek Allard's first assessment that it's incorrect to pass it nothing, i.e. the sample code in the report is using Active Record incorrectly. The behavior change has obviously impacted you in a negative way, though, so we're fine with a happy medium, and recognize that there's some redundant code that made it in with the NULL checks, etc. With CodeIgniter, we sometimes don't get the immediate feedback on testing that we get in a commercial application environment where we can do targeted beta testing. This makes small mistakes in CI seem larger than they really are, and more frustrating for both the developers and the end users.

Speaking frankly, the attitude of some people in this thread has discouraged us from participating in the discussion with the community. It's incredibly easy when you encounter a problem after the fact to identify it in the code and then have an aggressive "why did this make it in here?" posture. But if you really want to have a positive impact on CodeIgniter, you could instead help us out by running at least a local dev site off the svn instead of sitting back and refusing to run the current version or svn. Bugs will happen, it's a fact of life, and though it didn't affect a single line of code that D'Allard or I used, it could have been caught by any participant in this thread before release. D'Allard and I both have multiple CI svn installations in separate environments that we use to test before release, how many do you have? It's worth mentioning that this behavior change didn't impact our development code of ExpressionEngine 2.0 at all, so the larger difference here is how people are viewing the expected behavior from AR, not sloppy or untested code.

[eluser]Derek Allard[/eluser]
Hello all.

I've upload a behavioural change into the SVN. I'd greatly appreciate your input.

[eluser]Armchair Samurai[/eluser]
Derek A: So far, so good - the SVN version seems to be working for me. Thanks for the update.

[eluser]PV-Patrick[/eluser]
I tried the SVN version and a couple other DB functions I had using set() + insert() didn't work.... However, they work with the current 1.6.2 release.

Note that with the current 1.6.2 the set method vs array works fine with null/empty values.

[eluser]Derek Allard[/eluser]
Patrick, I'm not able to recreate. My set() tests with insert work. Can anyone else verify?

[eluser]PV-Patrick[/eluser]
I'll double check my code and try it again...

Just for reference: http://dev.ellislab.com/svn/CodeIgniter/...ve_rec.php

I am using the above code + PayPal IPN. Basically setting what variables I need from the ipn_data() and inserting to a DB. I'll let you know...

[eluser]PV-Patrick[/eluser]
Derek,
I just tried it both ways, using the array for insert and the set method using the file listed above for the active record class.

I didn't receive any errors in the log file for the query, however nothing was posted to the DB. I received a successful transaction postback from paypal via the log and nothing on db insert.