Possible bug: Model functions (doDelete, doFirst, etc..) ignores zero as id

[ECode16]

PHP Version

8.3

CodeIgniter4 Version

4.1.6

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Linux

Which server did you use?

apache

Database

No response

What happened?

doDelete from Model.php assumes there already is where query when $id is falsy value. I get why null and empty arrays are ignored but why zeros as well? (empty string as primary key is some black magic fuckery from my nightmares thought)

if (! in_array($id, [null, '', 0, '0', []], true)) {
    $builder = $builder->whereIn($this->primaryKey, $id);
}

Steps to Reproduce

in model $this->delete(0);

Expected Output

deletion of row with primary key 0

Anything else?

The only related thing I found in user guide is validation rule in Using CodeIgniter’s Model 'id' => 'max_length[19]|is_natural_no_zero'. Tried some googling if there's reason for zero to be invalid ID but only found some question about why auto-increments start at 1.

There was an attempt to modify this behaviour back in 2021 but it went stale and then closed.

It was introduced in 4.0.5 as implicit conversion of $id to bool and before the check was:

if (! empty($id) && (is_numeric($id) || is_string($id)))
{
    $id = [$id];
}

Sorry if I missed something already written about this problem.

[ECode16]

Also BaseModel delete has similar check if (! in_array($id, [null, 0, '0'], true) && (is_numeric($id) || is_string($id))) { but both will allow float 0.0 as an id.

and BaseModel update uses only if (is_numeric($id) || is_string($id)) {

[michalsn]

Honestly, I've never seen a production system use 0 or an empty string as a primary key value - though it’s technically allowed in most databases (except for '' in Oracle, where it’s treated as NULL).

While 0 can be valid and occasionally used for reserved or system-level rows, and '' might appear in edge-case designs, both patterns are generally discouraged. They can introduce ambiguity in application logic (especially in languages like PHP, , where 0 is treated as "empty") and reduce clarity in data modeling.

That said, I'm happy to hear other opinions. Should we actually "fix" this to allow values like 0? Or make the check more like how empty() works in PHP? I'm open to ideas if there's a good reason for supporting this.

[ECode16]

I admit I haven't seen empty string or zero pk in production either. Only found out through some edge case tests.

I'm not sure whether zero should be supported or not (I would leave that question to someone more experienced) but I think it should at least be identical to treatment of 0.0 float.

[neznaika0]

I was making a change. We can't just check if ($id) - the ID has an extended type: array|int|string. Therefore, it was decided to ban all invalid values. The ID can be 0 or " only in a single case. You can make a mistake with an empty string or converting the type to 0. But float is just as unacceptable as an empty string.

Regarding update(): There was NO goal to update the logic - it was necessary to adapt to phpstan. If you wish, you can unify all the checks for $id in the new PR. Because we don't take bool into account 😞

[crustamet]

we should define the primary_key_type as default to 'string' in the Model to fix this easily :) if you want you could change that to anything else

and the checker should check based on type if string check if it is a string etc..

But the real deal is you can have a table with no primary key from the start

and when you delete an record or update it

it should DELETE FROM etc.. WHERE columns = value AND column1 = value etc..

[neznaika0]

If you look at the commit history, you will see that this was the case before. if ($id) ... It did not allow empty values. Therefore, for now everything is as before. @michaln noted that $lastID = 0 and it would be wrong for ID.

If you have created erroneous records with ID <0, you should run SQL directly through exec() to fix them. The builder is not needed here