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

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

Closed
nowackipawel opened this issue Feb 7, 2019 · 4 comments
Closed

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

nowackipawel opened this issue Feb 7, 2019 · 4 comments

Comments

@nowackipawel
Copy link
Contributor

nowackipawel commented Feb 7, 2019

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

nowackipawel commented Feb 7, 2019

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

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.

@jim-parry jim-parry added this to the 4.0.0-beta.2 milestone Mar 5, 2019
@atishhamte
Copy link
Contributor

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

@lonnieezell
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants