CodeIgniter4 icon indicating copy to clipboard operation
CodeIgniter4 copied to clipboard

Bug: [Model] Limitation in Boolean Support (true/false) in `update()`/`save()`/`insert()`

Open datamweb opened this issue 1 year ago • 4 comments

PHP Version

8.3

CodeIgniter4 Version

v4.5.4

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Windows

Which server did you use?

apache

Database

No response

What happened?

$this->update($messageId, [
  'is_pinned'     => false,
]);
 ------ ------------------------------------------------------------------------------------------------------
  Line   src\Models\MessageModel.php
 ------ ------------------------------------------------------------------------------------------------------
  107    Parameter #2 $row of method Datamweb\BlankPackage\Models\BaseModel::update() expects array<int|string,
         float|int|object|string|null>|object|null, array<string, int|false|null> given.
 ------ ------------------------------------------------------------------------------------------------------

Currently, CodeIgniter 4 does not support boolean values (true and false) in models due to restrictions in the PHPStan typing declaration (@phpstan-type row_array). https://github.com/codeigniter4/CodeIgniter4/blob/153922e60fd9ec61c5799afd7d8328a4f382716e/system/BaseModel.php#L50 This definition only allows int, float, null, string, and object types, effectively preventing boolean values from being used in database operations.

This issue is particularly problematic because certain database drivers natively support boolean values, and they should be allowed for use directly.

Database Driver Details

Database Boolean Support Data Type Accepted Values
MySQL Yes TINYINT(1) true / false (1/0)
PostgreSQL Yes BOOLEAN true / false
SQL Server Yes BIT true / false (1/0)
SQLite No (requires conversion) INTEGER 1 / 0
OCI8 (Oracle) No (requires conversion) NUMBER 1 / 0

Steps to Reproduce

To address this limitation and allow boolean values, it is suggested to:

  1. Update the PHPStan-type declaration to support boolean values (true and false) for all supported databases.
* @phpstan-type row_array               array<int|string, float|int|null|object|string|bool> 
  1. Add automatic conversion for databases that do not support boolean values directly (like SQLite and OCI8) by converting true to 1 and false to 0.

Example Code for Handling Boolean Conversion

protected function prepareDataForSQLiteAndOCI8(array $data): array
{
    $dbDriver = $this->db->DBDriver;

    if ($dbDriver === 'SQLite3' || $dbDriver === 'OCI8') {
        foreach ($data as $key => $value) {
            if (is_bool($value)) {
                $data[$key] = $value ? 1 : 0;
            }
        }
    }

    return $data;
}

These changes will ensure better compatibility with all database drivers, allowing proper handling of boolean values in databases that do not natively support them.

Expected Output

Manually casting boolean values is a workaround and places unnecessary burden on developers. CodeIgniter 4 should handle these conversions internally, especially given that:

Many database drivers natively support boolean values (true and false). The framework should abstract away such differences, allowing developers to focus on business logic rather than database-specific nuances.

In conclusion, while manual casting (e.g., (int)false OR 1/0) solves the problem for now, this limitation should not exist, as proper support for boolean values will make the framework more robust and user-friendly.

Anything else?

No response

datamweb avatar Sep 20 '24 17:09 datamweb

It seems bool is missing in the PHPDoc type.

kenjis avatar Sep 24 '24 03:09 kenjis

If only PHPDoc is on the way to supporting booleans, then that's a good thing to change.

I'm not a big fan of point 2. This would mean that each write query would be subject to an additional loop check of all passed arguments. Seems like a big waste of time/resources - for this one thing.

If we want "magic" we have Entities or Data conversion in the model. When we use Query Builder we're responsible for the values we use/allow. If a certain database does not support booleans, it's not a framework's fault.

michalsn avatar Sep 25 '24 08:09 michalsn

Data conversion in the model.

@michalsn So, you prefer something similar to the code below and believe the user should adapt it for compatibility like this?

    // Hooks before insert and update
    protected $beforeInsert = ['prepareDataForSQLiteAndOCI8'];
    protected $beforeUpdate = ['prepareDataForSQLiteAndOCI8'];

    /**
     * Prepare data before saving or updating.
     * This method adjusts 'is_pinned' field for specific database drivers like SQLite and OCI8.
     *
     * @param array $data The data being inserted or updated
     * @return array Modified data
     */
    private function prepareDataForSQLiteAndOCI8(array $data): array
    {
        // Get the database driver
        $dbDriver = $this->db->DBDriver;

        // Adjust 'is_pinned' for SQLite and OCI8 drivers
        if (($dbDriver === 'SQLite3' || $dbDriver === 'OCI8') && isset($data['data']['is_pinned'])) {
            $data['data']['is_pinned'] = $data['data']['is_pinned'] ? 1 : 0;
        }

        return $data;
    }

datamweb avatar Sep 27 '24 14:09 datamweb

@datamweb I was thinking more about model field casting, although I don't remember if we have access to a database object there.

michalsn avatar Sep 27 '24 15:09 michalsn

I agree with extending the bool type in PHPDoc to address this issue, as CodeIgniter4 provides entities and data conversion features that effectively handle such cases.

Additionally, I have submitted a pull request in #9276.

ping-yee avatar Nov 13 '24 14:11 ping-yee