The getObjectModel() method should work for all ActiveRecord classes

[marc-farre]

What steps will reproduce the problem?

class A extends ActiveRecord
{}

class B extends class A
{
    public static function getObjectModel(): string
    {
        return A::class;
    }
}

Attach a file to B

$a = new A();
count ($a->fileManager->findAll());

What is the expected result?

1 (the file is attached to A).

What do you get instead?

0 (the file is attached to B).

Additional info

This is because PolymorphicRelation::getObjectModel() only works for ContentActiveRecord or ContentAddonActiveRecord:
https://github.com/humhub/humhub/blob/master/protected/humhub/components/behaviors/PolymorphicRelation.php#L119

@luke- would you accept a PR where:

    public static function getObjectModel(): string
    {
        return static::class;
    }

is moved from ContentActiveRecord and ContentAddonActiveRecord to ActiveRecord?

And in PolymorphicRelation::getObjectModel() I'll replace:

return $object instanceof ContentActiveRecord || $object instanceof ContentAddonActiveRecord

with:

return $object instanceof ActiveRecord

Q	A
HumHub version	1.15.2

[ArchBlood]

It's not really a common approach for extending multiple classes no?

Normally you'd use PHP magic methods __call() or __callStatic() in these cases. 🤔

[marc-farre]

It's not really a common approach for extending multiple classes no?

Use case:

class Product extends ActiveRecord {...}
class ProductForm extends Product {...}

I use ProductForm to attach images.

[ArchBlood]

It's not really a common approach for extending multiple classes no?

Use case:

class Product extends ActiveRecord {...}
class ProductForm extends Product {...}

I use ProductForm to attach images.

In the FAQ we simply use the following;

Ticket

class Ticket extends ContentActiveRecord implements Searchable {...}

TicketSearch

class TicketSearch extends Ticket {...}

Then set scenarios() where we call Model::scenarios(); and search() which queries Ticket variables, otherwise. 🤔

I also do the same method for the Faq model;

class Faq extends ActiveRecord {...}

class FaqSearch extends Faq {...}

[luke-]

What steps will reproduce the problem?

class A extends ActiveRecord
{}

class B extends class A
{
    public static function getObjectModel(): string
    {
        return A::class;
    }
}

Attach a file to B

$b = new B();
count ($b->fileManager->findAll());

What is the expected result?

1 (the file is attached to B).

What do you get instead?

0 (the file is attached to A).

@marc-farre Are you sure your example here isn't reversed?

I haven't tested it, but I would expect files from B are returned, but you want files from A (As you implemented in the not used getObjectModel().

---

In general, I'm not a big fan of the getObjectModel() method as it's hard to understand what the implications are.

Since it seems only relevant regarding PolymorphicRelations, I am thinking about an interface like PolymorphicRelationTargetInterface with a method getPolymorphicRelationObjectModel(): string and implement this in ActiveRecord.

And PolymorphicRelation::getObjectModel() can use this method.

What do you think about this?

@yurabakhtin What do you think?

[marc-farre]

@luke- yes, sorry, I've updated my example (below, a more understandable use case).

I agree, this getObjectModel() adds complexity and can generate errors in the DB if we omit to add it in classes extending ActiveRecord classes.

But in another hand, I think we should have the possibility to extend any ActiveRecord classes without breaking the polymorphic relation because, at least for the file table, any ActiveRecord can attach files in it.

So I think that restricting getObjectModel() to only ContentActiveRecord or ContentAddonActiveRecord adds even more complexity than having this method working for all ActiveRecord.

PolymorphicRelationTargetInterface

Yes, the PolymorphicRelationTargetInterface would be much better I think.

Another approach, more complicated in the code, but that could avoid getObjectModel() or PolymorphicRelationTargetInterface (so maybe more reliable for new developers) would be, in PolymorphicRelation::getObjectModel(), to find the closer parent class declaring the tableName() method (because this is the class that should be used for polymorphic relations).

It could be done this way (not tested):

function getCloserParentClassWithTableName($object) {
    $class = get_class($object);
    if (method_exists($class, 'tableName')) {
        return $class;
    }
    while ($class = get_parent_class($class)) {
        if (method_exists($class, 'tableName')) {
            return $class;
        }
    }
    return NULL;
}

$b = new B();
echo getCloserParentClassWithTableName($d); // 'A'

A more understandable use case:

class Product extends ActiveRecord
{}

class ProductForm extends Product
{
    public static function getObjectModel(): string
    {
        return Product::class;
    }
}

A form allows attaching files to ProductForm.

The problem is that in the table file, object_model stores humhub\modules\myModule\models\ProductForm instead of humhub\modules\myModule\models\Product

[luke-]

But in another hand, I think we should have the possibility to extend any ActiveRecord classes without breaking the polymorphic relation because, at least for the file table, any ActiveRecord can attach files in it.

So I think that restricting getObjectModel() to only ContentActiveRecord or ContentAddonActiveRecord adds even more complexity than having this method working for all ActiveRecord.

@marc-farre I agree with that. Since the File module is based on polymorphic relations, we must have this option at the ActiveRecord level.

PolymorphicRelationTargetInterface

Yes, the PolymorphicRelationTargetInterface would be much better I think.

Another approach, more complicated in the code, but that could avoid getObjectModel() or PolymorphicRelationTargetInterface (so maybe more reliable for new developers) would be, in PolymorphicRelation::getObjectModel(), to find the closer parent class declaring the tableName() method (because this is the class that should be used for polymorphic relations).

Interesting idea. To summarize: Always use the class in hierarchy which directly implements the tableName() method? But I'm not sure this is possible, because method_exists should always return true, because there is a very early tableName() implementation of the Yii ActiveRecord implemenation.

Hmm, actually, we can also keep the name getObjectModel(). The PHPDoc is quite good and we could easily add an interface at any time. So from my point of view, I would be fine to simply move it to AR...

[marc-farre]

@luke- yes, you're right (as usual 😉!). But it's possible to detect in which class the tableName() method has been declared this way (tested successfully this time!):

class Product extends ActiveRecord
{
    public static function tableName()
    {
        return 'product';
    }
}

class ProductForm extends Product
{
    public static function getObjectModel(): string
    {
        return Product::class;
    }
}

In PolymorphicRelation class, something like:

        function tableNameIsDeclared($className)
        {
            $reflector = new \ReflectionClass($className);
            $method = $reflector->getMethod('tableName');
            return $method->class === $className;
        }


        function getObjectModel($object)
        {
            $class = get_class($object);
            if (tableNameIsDeclared($class)) {
                return $class;
            }
            while ($class = get_parent_class($class)) {
                if (tableNameIsDeclared($class)) {
                    return $class;
                }
            }
            return null;
        }

        $object = new ProductForm();
        echo PolymorphicRelation::getObjectModel($object); // returns humhub\modules\myModule\models\Product

So should we implement this and remove getObjectModel() everywhere (except the PolymorphicRelation of course), or should move getObjectModel() to the AR level?

[luke-]

Hmm, I would prefer to move "getObjectModel()" to AR.

The other solution is also interesting. The only question is whether the class with "tableName()" is really always the right one. And if not, how could it be overwritten?

[ArchBlood]

The other solution is also interesting. The only question is whether the class with "tableName()" is really always the right one. And if not, how could it be overwritten?

Wouldn't using tablePrefix come into play here for overwriting the existing table(s)? Personally speaking if you have a model that extends the AR class that has tableName() defined, and then you extend that class, you should either use a tablePrefix or a completely new tableName(), as for checking whether or not the class is really always the right one, I believe this would be then defined in the functions implemented for each class wouldn't it?

Example

<?php

namespace app\models;

use yii\db\ActiveRecord;

class BaseModel extends ActiveRecord
{
    public static function tableName()
    {
        return 'default_table_name';
    }
}

<?php

namespace app\models;

class CustomModel extends BaseModel
{
    public static function tableName()
    {
        return 'custom_default_table_name';
    }
}

Another method would be

<?php

namespace app\models;

use yii\db\ActiveRecord;

class BaseModel extends ActiveRecord
{   
    public static function tableName()
    {
        return 'default_table_name';
    }
}

<?php

namespace app\models;

class CustomModel extends BaseModel
{
    // Your custom implementation
    
    public static function getTablePrefix()
    {
        // You can dynamically determine the table prefix here
        return 'dynamic_prefix_';
    }
    
    public static function tableName()
    {
        return static::getTablePrefix() . 'default_table_name';
    }
}

[marc-farre]

@luke- PR #6854