codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.35k stars 1.9k forks source link

Bug: CodeIgniter model not recognizing custom deletedField in find queries #3999

Closed mihail-minkov closed 3 years ago

mihail-minkov commented 3 years ago

Describe the bug I am using custom names for the created, modified and deleted fields of the model.

What happens is that using timestamps and soft deletes works perfectly. The correct fields get updated.

The problem comes that if I don't specify ->where('fecha_eliminacion', NULL) the ->findAll() function returns everything including deleted items. I checked the documentation and findAll() is supposed to ignore deleted items. This can be found here where the withDeleted() function is mentioned.

Below is an example of how I configure them. BaseModel only initiates the $this->db variable and NoticiasModel inherits it.

class NoticiasModel extends BaseModel
{
    public function __construct()
    {
        parent::__construct();

        $this->table = 'Noticias';
        $this->primaryKey = 'id_noticia';

        $this->createdField  = 'fecha_creacion';
        $this->updatedField  = 'fecha_actualizacion';
        $this->deletedField  = 'fecha_eliminacion';

        $this->useSoftDeletes = true;
        $this->useTimestamps = true;
    }
}

CodeIgniter 4 version 4.0.4 The entry in composer.json is "codeigniter4/framework": "^4@rc"

Affected module(s) Models

Expected behavior, and steps to reproduce if appropriate What I expect is the findAll() function to return everything except deleted items, but it actually returns everything including deleted items. I think it's not considering the custom field name correctly.

Context

iRedds commented 3 years ago

Why are you doing this? Why you change class properties in constructor?

Set default class property like

class NoticiasModel extends BaseModel
{
    protected $table = 'Noticias';
    protected $primaryKey = 'id_noticia';

    protected $createdField  = 'fecha_creacion';
    protected $updatedField  = 'fecha_actualizacion';
    protected $deletedField  = 'fecha_eliminacion';

    protected $useSoftDeletes = true;
    protected $useTimestamps = true;
}

Or call parent::__construct() after changing properties.

paulbalandan commented 3 years ago

It is not incorrect to set child properties in the constructor, though.

The problem here is when and where CodeIgniter\Model's protected $tempUseSoftDeletes is initialized by the setting defined by the child class's (in your case, NoticiasModel) $useSoftDeletes property. In the Model's constructor, $this->tempUseSoftDeletes is set by $this->useSoftDeletes, which by default is false. Since you are calling first the parent's constructor then afterwards setting NoticiasModel::$useSoftDeletes to true, the value passed to Model::$tempUseSoftDeletes is always false. This $tempUseSoftDeletes is crucial since this adds the extra WHERE statement you're looking for.

if ($this->tempUseSoftDeletes)
{
    $builder->where($this->table . '.' . $this->deletedField, null);
}

As a fix for this, you just need to call the parent constructor after setting all properties in NoticiasModel.

 class NoticiasModel extends BaseModel
 {
     public function __construct()
     {
-       parent::__construct();
-
         $this->table = 'Noticias';
         $this->primaryKey = 'id_noticia';

         $this->createdField  = 'fecha_creacion';
         $this->updatedField  = 'fecha_actualizacion';
         $this->deletedField  = 'fecha_eliminacion';

         $this->useSoftDeletes = true;
         $this->useTimestamps = true;
+
+        parent::__construct();
    }
}
mihail-minkov commented 3 years ago

Just to reaffirm what you explained @paulbalandan . So basically once I run parent::__construct() I can't override the BaseModel protected variables?

paulbalandan commented 3 years ago

Can you share your BaseModel's constructor?

mihail-minkov commented 3 years ago

@paulbalandan here it is:

<?php namespace App\Models;

use CodeIgniter\Model;

class BaseModel extends Model
{
    protected $db;

    public function __construct() {
        parent::__construct();

        $this->db = \Config\Database::connect();

    }
}

Should I move parent::__construct(); after $this->db... ?

I haven't had trouble with my BaseModel. As for the change you suggested that worked in the other models. Now I don't need to add the fecha_eliminacion condition to my queries.

iRedds commented 3 years ago

By default the Model already automatically creates the $db class property with a database connection. Example v4.0.2

class Model
{
    /**
     * Database Connection
     *
     * @var ConnectionInterface
     */
    protected $db;

    /**
     * Model constructor.
     *
     * @param ConnectionInterface $db
     * @param ValidationInterface $validation
     */
    public function __construct(ConnectionInterface &$db = null, ValidationInterface $validation = null)
    {
        if ($db instanceof ConnectionInterface)
        {
            $this->db = & $db;
        }
        else
        {
            $this->db = Database::connect($this->DBGroup);
        }

    }

If BaseModel constructor only contains connection creation, remove the constructor. Else just call parent::__construct();