Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mihail-minkov opened this issue Dec 17, 2020 · 6 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@mihail-minkov
Copy link

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

  • OS: Windows 10 Enterprise x64 20H2
  • Web server: Apache 2.4.25, but the site is running on php spark serve
  • PHP version: 7.4.11
@mihail-minkov mihail-minkov added the bug Verified issues on the current code behavior or pull requests that will fix them label Dec 17, 2020
@iRedds
Copy link
Collaborator

iRedds commented Dec 17, 2020

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
Copy link
Member

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
Copy link
Author

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

@paulbalandan
Copy link
Member

Can you share your BaseModel's constructor?

@mihail-minkov
Copy link
Author

mihail-minkov commented Dec 21, 2020

@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
Copy link
Collaborator

iRedds commented Dec 21, 2020

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();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

No branches or pull requests

3 participants