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

fix: add dbgroup to model template only when specified as an option #8077

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

sammyskills
Copy link
Contributor

Description
The property $DBGroup should only be added to the generated model template when specified as an option: --dbgroup.
Fixes #8073

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • [] Unit testing, with >80% coverage
  • [] User guide updated
  • Conforms to style guide

Copy link
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I personally prefer $DBGroup to be like this when creating the model.

protected $DBGroup = null;

The reason for this is in the visibility of this property for rapid change during development.
However, this is a personal opinion.

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 23, 2023
@kenjis
Copy link
Member

kenjis commented Oct 23, 2023

@datamweb I don't know what you exactly means by rapid change during development, but I feel strange that writing protected $DBGroup = null; in the model by default.

And more than 90% devs use single db group, I guess.
And, if a dev changes it to 'default' by mistake, tests will not work like the reported issue.

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine, but wait for other people's opinions.

@kenjis kenjis changed the title add dbgroup to model template only when specified as an option fix: add dbgroup to model template only when specified as an option Oct 23, 2023
@datamweb
Copy link
Contributor

@datamweb I don't know what you exactly means by rapid change during development, but I feel strange that writing protected $DBGroup = null; in the model by default.

Imagine that the model is make as follows.

<?php

namespace App\Models;

use CodeIgniter\Model;

class Sample extends Model
{
    protected $table            = 'Sample';
    protected $primaryKey       = 'id';
    protected $useAutoIncrement = true;
    protected $returnType       = 'array';
    protected $useSoftDeletes   = false;
    protected $protectFields    = true;
    protected $allowedFields    = [];

    // Dates
    protected $useTimestamps = false;
    protected $dateFormat    = 'datetime';
    protected $createdField  = 'created_at';
    protected $updatedField  = 'updated_at';
    protected $deletedField  = 'deleted_at';
}

Do you realize that there are Validation or Callbacks? No, you must see the documents.

<?php

namespace App\Models;

use CodeIgniter\Model;

class Sample extends Model
{
    protected $table            = 'Sample';
    protected $primaryKey       = 'id';
    protected $useAutoIncrement = true;
    protected $returnType       = 'array';
    protected $useSoftDeletes   = false;
    protected $protectFields    = true;
    protected $allowedFields    = [];

    // Dates
    protected $useTimestamps = false;
    protected $dateFormat    = 'datetime';
    protected $createdField  = 'created_at';
    protected $updatedField  = 'updated_at';
    protected $deletedField  = 'deleted_at';

    // Validation
    protected $validationRules      = [];
    protected $validationMessages   = [];
    protected $skipValidation       = false;
    protected $cleanValidationRules = true;

    // Callbacks
    protected $allowCallbacks = true;
    protected $beforeInsert   = [];
    protected $afterInsert    = [];
    protected $beforeUpdate   = [];
    protected $afterUpdate    = [];
    protected $beforeFind     = [];
    protected $afterFind      = [];
    protected $beforeDelete   = [];
    protected $afterDelete    = [];
}

As a beginner in CodeIgniter, I prefer to have everything in front of my eyes so that I can be aware of the existence of this feature and be curious. Not that I have to constantly read the documentation or even not know about many features. This is very important for me, who is not an English speaker.

And more than 90% devs use single db group, I guess.
And, if a dev changes it to 'default' by mistake, tests will not work like the reported issue.

I agree with you on this.

@paulbalandan
Copy link
Member

I prefer to have everything in front of my eyes so that I can be aware of the existence of this feature and be curious. Not that I have to constantly read the documentation or even not know about many features. This is very important for me, who is not an English speaker.

For me, I don't want code to clutter when I don't have a need for them. Just the needed code to run.

@kenjis kenjis merged commit b56c85c into codeigniter4:develop Oct 25, 2023
61 checks passed
@kenjis
Copy link
Member

kenjis commented Oct 25, 2023

@sammyskills Thank you!

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

Successfully merging this pull request may close these issues.

Bug: Models declared in the App namespace references the 'default' database group in Tests.
4 participants