Allow to instantiate the called class instead of the CActiveRecord class

[eduardor2k]

The problem of this implementation is that this method needs to copied onto the children class to be instantiated correctly, since if the method is inherited _ _ CLASS _ _ will always point to CActiveRecord instead of the class calling this method, using get_called_class() fixes that, this function is present since php 5.3.0

Here's an example:

class A extends CActiveRecord {}

And we call A::model(), we will get an A instance instead of CActiveRecord

<?php

class B
{
    public static function model($className = __CLASS__)
    {
        return new $className(null);
    }	
}

class A extends B
{
}

$b = new B();
$a = new A();

var_dump($b::model());
var_dump($a::model());

returns

object(B)#3 (0) { } 
object(B)#3 (0) { } 

but if we change that to get_called_class()

<?php

class B
{
    public static function model($className = null)
    {
        if($className === null){
	     $className = get_called_class();
        }
        return new $className(null);
    }	
}

class A extends B
{
}

$b = new B();
$a = new A();

var_dump($b::model());
var_dump($a::model());

results in

object(B)#3 (0) { } 
object(A)#3 (0) { }

Q	A
Is bugfix?	❌
New feature?	❌
Breaks BC?	✔️
Tests pass?	✔️

[what-the-diff]

PR Summary

• Enhancement of the model Method in CActiveRecord.php
  The function named model in the file CActiveRecord.php has been updated. Presently, it can take in a parameter labeled className that might be null. If this parameter does not have any assigned value (null), then the method assigns a default value coming from a function called get_called_class(). This improvement enables the system to dynamically decide the class name during the execution of the model method, increasing its flexibility.

[Arhell]

@marcovtwout

[twisted1919]

I don't think this should change, we've already accepted this in Yii1, we're all used to it, makes no sens to change now.

[eduardor2k]

I don't think this should change, we've already accepted this in Yii1, we're all used to it, makes no sens to change now.

you mean, having to clone the model method in every model, to get the correct instance?

I'm sure you everybody has done this in all their yii1 projects, this change, should be backwards compatible with those cases.

@twisted1919 what's your use case for this function?

Btw: I don't want to start a war, since this can be fixed in user code, you can extend the CActiveRecord class and have this method there and use that to extend your model.

[twisted1919]

Yeah, everybody had the copy of the method already because that's what Gii generates, that was my point, it is really a non-problem, all the models already have that when they are generated by Gii with the default template.

What stops you to extend CActiveRecord, implement your change in that class, then extend your models from it?

[twisted1919]

what's your use case for this function?

I just extended CActiveRecord as I said earlier and for some model classes I do have some custom code, but in general it's just overriding the parent implementation to get the right model class, nothing too fancy. In every model, I know, lots of repetition, but i've done it so many times, I simply ignore it now.

Just to be on the same page, I have nothing against your change per-se, I just think we're already way too used to have that method in each model that at this point it doesn't matter that much to me if the ergonomics are improved.

[eduardor2k]

@twisted1919 you're totally right, my reasoning is I don't see the point to keep it that way, having that code added to every model is pointless (literally), because the finality of the model() method is to get the instance of your model, not the CActiveRecord, it appears this comes from the days of yii 1.0, when get_called_class() did not exist and I'm not sure if there was a workaround around this back then (prior to php 5.3)

I would extend the scope of this, updating the model template and getting rid of the model method

better late than never

PS: Thanks @twisted1919 for giving your point of view

[twisted1919]

I would extend the scope of this, updating the model template and getting rid of the model method

plus updating the docs, I think the use case is heavy documented, I might be wrong though.

I defer to @marcovtwout after all, whatever he thinks. It doesn't change things for me as long as there's no BC, if it makes your life easier, sure.

[dimvic]

This is not a fully BC change.
If one has explicitly not included the method in their models to do something exotic, by exploiting the difference between static and self for example, this could be a breaking change.
Does not look like a real world scenario nor would that be good practice, but the case exists.

[eduardor2k]

Does not look like a real world scenario nor would that be good practice, but the case exists.

True

PS: I've set, "Breaks BC" to Yes in the top post

[marcovtwout]

It's a nice idea, but for Yii 1 it wouldn't make sense anymore since the current implementation is well established and documented. Changing this now, even in a backward compatible manner, would only cause confusion.