alpha4->5 requires to have primary key in every model/table

[nowackipawel]

Changes in Models classToArray and crud methods starts to require every model to have defined primaryKey. In some occasions i.e. storage-like tables (where you don't want to look often like - login attempts) don't use primary keys and that case causes exception:

SYSTEMPATH/Entity.php : 289   —  CodeIgniter\Debug\Exceptions->errorHandler ( arguments )

282         }
283         // Or cast it as something?
284         else if ($this->_cast && isset($this->_options['casts'][$key]) && ! empty($this->_options['casts'][$key]))
285         {
286             $result = $this->castAs($result, $this->_options['casts'][$key]);
287         }
288 
289         return $result;
290     }

SYSTEMPATH/Model.php : 577   —  CodeIgniter\Entity->__get ( arguments )

574             // Always grab the primary key otherwise updates will fail.
575             if (! empty($properties) && ! empty($pk) && ! in_array($pk, $properties))
576             {
577                 $properties[$pk] = $data->$pk;
578             }

SYSTEMPATH/Model.php : 669   —  CodeIgniter\Model::classToArray ( arguments )

669             $data = static::classToArray($data, $this->primaryKey, $this->dateFormat);

I think this is step back :(.

My model:

<?php

class UserLoginAttemptModel  extends \CodeIgniter\Model
{
    protected $DBGroup    = DB_ZTN_GROUP;
    protected $table      = 't_user_login_attempt';
    protected $returnType = 'App\Entities\User\UserLoginAttempt';

    protected $allowedFields = [
        'usl_usr_id',
        'usl_ip',
        'usl_is_successful',
    ];
}

and Entity:

<?php namespace App\Entities\User;

use CodeIgniter\Entity;

class UserLoginAttempt extends Entity
{
    protected $usl_usr_id;
    protected $usl_ip;
    protected $usl_is_successful;
    protected $usl_created_at;

    protected $_options = [
        'casts'   => [
            'usl_usr_id'        => 'integer',
            'usl_ip'        => 'integer',
            'usl_is_successful' => 'boolean',
        ],
        'dates'   => ['usl_created_at'],
        'datamap' => [],
    ];

    public function setAulIp(string $value)
    {
        $this->usl_ip = inet_pton($value);
    }

    public function getAulIp()
    {
        return inet_ntop($this->usl_ip);
    }

}

I don't want to judge correctness of using tables without primary keys but it is allowed to have one or two... that's why we should not force devs to use primaryKeys everywhere ... even if this is not necessary. Additional this change keeps us far from availability to use complex primary keys like : $primaryKey = ['first_field','second_field'];

[nowackipawel]

Btw: my lines in Model could be sligtly different because I extended my Model directly in model class, but it does not interfere with this case at all and it is merged with the newest dev version.

[lonnieezell]

A couple of things here. In every abstraction there are certain conventions that end up being required. That's the nature of programming. We've done our best to make this as flexible as possible and still provide tools that your life easier than they were before. While some of the features do assume a primary key, like Entities, there are still plenty of other ways to achieve what you need with the framework.

While I'll take a look at that specific situation to see if there's anything that can be made more flexible and maintain functionality, there are times when a developer needs to be flexible. Because it's a whole lot less work for you to write a migration that adds a primary key to your tables than it is for me to ensure the entire system works for this 1% use case.

And I get why you might want tables like that, I do. In my day job, though, we've faced a similar situation with Laravel and the choices for us were to either fight the framework to get some features to work right, or just add a primary key to all tables and not worry about it again... I bet you can guess which way we chose.

[atishhamte]

w.r.t #1829, this is valid case that every model should have primary key.

[lonnieezell]

@atishamte And it's this issue that prompted me to examine things and finally decide we should enforce that. :)