lonnieezell / Bonfire2

CodeIgniter 4-based application skeleton
MIT License
127 stars 49 forks source link

Make syncMeta work with classes other than Users #428

Closed dgvirtual closed 4 months ago

dgvirtual commented 4 months ago

Adds fix for #426. Please review, maybe there is a more automatic way of doing it (like passing host class parameter to the trait?).

Also I am not sure if setting a default is the way to go:

$configClass ??= 'Users';

(And event then there is a smarter way to pass the default, right in method parameter declaration... will fix that but later, please review first, if the general approach is ok).

lonnieezell commented 4 months ago

You're probably going to want to throw rocks at me or something lol

After seeing how it looks here, I think it would make more sense to define the config file on the class that has the trait. It looks odd when calling $user->syncMeta('Config\User') since it feels like the $user should already know. Also, it would be a source of errors if all devs were required to pass in the config file each time.

dgvirtual commented 4 months ago

You're probably going to want to throw rocks at me or something lol

Actually it is good to know devs with much more experience do not always get it right the first time they look at the code :)

After seeing how it looks here, I think it would make more sense to define the config file on the class that has the trait. It looks odd when calling $user->syncMeta('Config\User') since it feels like the $user should already know. Also, it would be a source of errors if all devs were required to pass in the config file each time.

I agree with you, this solution looks odd and invites errors. What you suggest, however, would mean renaming the config file and possibly breaking existing installs on update (if anyone is using meta fields with Users class). What would be the right way to update this? I guess automatic update of a file in app/Config would be risky, so, should there be some instruction on upgrading, as is done in Codeigniter itself, via release notes? However, we do not actually have releases in Bonfire2...

daracorcoran commented 4 months ago

After seeing how it looks here, I think it would make more sense to define the config file on the class that has the trait. It looks odd when calling $user->syncMeta('Config\User') since it feels like the $user should already know. Also, it would be a source of errors if all devs were required to pass in the config file each time.

Maybe I'm wrong but my interpretation of this is that, for example, an entity would look like

class User extends Entity
{
using HasMeta;
private string $configClass = 'Users';

and HasMeta would then reference $this->configClass. But it's probably not a good idea to have the trait so dependent on the parent class.

dgvirtual commented 4 months ago

and HasMeta would then reference $this->configClass. But it's probably not a good idea to have the trait so dependent on the parent class.

Yeh, my first interpretation was like yours, but I was also worried about this dependency.

So my second idea was to assume that the config class should have the same name as entity class (but different namespaces of course), and then automatically conjure the config class name via get_class($this)

Here is some test code I wrote that will explain if my English failed:


class MyClassConfig {
    public $name = 'Cat';
}

function config($className) {
    // to make this work within the same namespace I concatenate strings
    $configClassName = $className . 'Config';
    // with different namespaces (and same class name), I would call 
    // $configClassName = '\Config\'. $className;
    return (new $configClassName())->name;
}

trait GreetingTrait {
    public function traitGreet() {
        return "Hello, world " . config(get_class($this)) . PHP_EOL;
    }
}

class MyClass {
    use GreetingTrait;

    public function classGreet() {
        return "Hi there from " . config(get_class($this)) . PHP_EOL;
    }
}

$obj = new MyClass();
echo $obj->traitGreet();  // returns 'Hello world Cat'
echo $obj->classGreet(); // returns 'Hi there from Cat'

So, @lonnieezell , how should I proceed?

I guess the breaking change that this would require (a new config class User would have to appear, with property $metaFields moved from Users config class) could be solved by adding a section Upgrading in the manual and listing all the breaking changes chronologically with dates, that would help people get note when manual adjustments should be made.

daracorcoran commented 4 months ago

Another option would be an abstract method in the trait, to ensure the parent class will implement the method. Something like a getter for the config class. Not amazing, but there may be less refactoring involved. I'm not sure that this really "belongs" in an Entity, but that could be up for discussion.

Something like this:

trait HasMeta{

    abstract protected function _getConfigClass();

    public function messageFromTrait(){
        return "Config Class in trait: " . $this->_getConfigClass();
    }

}

class User{

    use HasMeta;

    private string $configClass = 'User';

    protected function _getConfigClass(){
        return $this->configClass;
    }

}

$user = new User();
echo $user->messageFromTrait();
dgvirtual commented 4 months ago

@lonnieezell, we clearly need your advice, which of these proposals should be implemented?