Bug: [Factories] when there are classes with the same short name, only the first class is loaded

[kenjis]

When there are classes with the same short name, and we try to load them with Factories by passing the full classname, only the first class is loaded.

There is no way to load the second class when specifying the full classname.

<?php

namespace App\Models;

use CodeIgniter\Model;

class Sample extends Model
{
    protected $table         = 'samples';
    protected $allowedFields = [];
}

<?php

namespace App\Models\Dir;

use CodeIgniter\Model;

class Sample extends Model
{
    protected $table         = 'dir_samples';
    protected $allowedFields = [];
}

<?php

namespace App\Controllers;

use App\Models\Dir\Sample as DirSample;
use App\Models\Sample;

class Home extends BaseController
{
    public function index()
    {
        $sample    = model(Sample::class);
        $dirSample = model(DirSample::class);

        var_dump($sample === $dirSample);
    }
}

The result is true.

Related #7684

[kenjis]

Of course we can load the second class if we use the relative path:

$dirSample = model('Dir/Sample');

https://codeigniter4.github.io/CodeIgniter4/concepts/factories.html#loading-a-class-in-sub-directories

But it cannot specify the namespace.

[paulbalandan]

I don't understand the reasoning behind this basenaming logic. This very logic causes the bug in the Debug Toolbar where passing config(static::class) does not error since the class and its config share the same basename, Toolbar.

See 6f8f3a2

[kenjis]

@paulbalandan I think it is for implementing preferApp feature.
https://codeigniter4.github.io/CodeIgniter4/concepts/factories.html#preferapp-option

[paulbalandan]

In my opinion, if an FQCN is passed it should be just check only against class_exists and self::verifyInstanceOf and return. If a basename is passed (i.e. no namespace separators, like App), it should be checked if existing and satisfies the preferApp option when added the Config\ or App\Models\ namespace.

[kenjis]

@paulbalandan That is easy to understand, and it does not surprise devs.
I thought like that in the first time.

But the current behavior is intentional from the begininng. See #5356

If we change it, it will be a big change.

[kenjis]

@MGatner Why do you need preferApp ?
I think it is needed for Config, because devs need to use their own Config instead of a Config file in a Composer package.
But is it really needed for Models or other classes?

The current behavior is that if a developer happens to have a class in App/Models with the same short name, that class will be loaded instead of the class in a Composer package.
If there is App/Models/GroupModel, Shield's GroupModel will not be used, and Shield will not work correctly.

preferApp assumes that there are no classes with the same short name, which seems uncertain.
Also, to PHP devs, it is hard to assume that a different class will be loaded by specifying the FQCN.

We don't know which namespace Foo is loaded by model('Foo'), and we have no control over it if they are all in Composer packages.

[kenjis]

I found another bug or unexpected behavior with preferApp. #7699

[iRedds]

Who can explain in a nutshell why we need the model() function?
As I see it, this is an implementation of a singleton. So why not make a static method on the model that will return the same class instance?

UserModel::model() === UserModel::model()

Or a function that will store instances of objects.

singleton(Some::class) === singleton(Some::class)

[MGatner]

I think the solution here is to introduce a check for parent classes. So model('Shield\Models\UserModel') will only return App\Models\UserMdoel if it extends the Shield version. This way full class names can still be used and expected to return the call or one of its children. See my reference to how I implement a similar thing in Tatter\Handlers.

[MGatner]

Who can explain in a nutshell why we need the model() function?

The function is just a convenient wrapper for Factories. It functions similarly to a singleton but the power of it is to allow third-party modules to be extended natively. So if Acme\Foo has Acme\Foo\Models\WidgetModel and uses model(WidgetModel::class) to load it's own model, then devs can provide an alternate WidgetModel in App to be used in its place, without extending services or modifying the vendor package in any way.

[iRedds]

@MGatner I could be wrong, but it seems to me that most of those who encounter the problem voiced here are using the model() function as a class loader from CI 3 (i.e. $this->load->model()), and not the ability to replace models for third party modules.

What you're talking about, I think it should be solved like this.

// in module
model(\Acme\Some::class); // alias \Acme\Some::class for class \Acme\Some::class
// in app
model(\Acme\Some::class, \App\MyClass::class); // alias \Acme\Some::class for class \App\MyClass::class

Any subsequent call to model(\Acme\Some::class) after model(\Acme\Some::class, \App\MyClass::class) will return the \App\MyClass::class instance. That is, implement a service container.

[kenjis]

@MGatner Who uses the feature to override the third-party models?
It is not clearly explained, so I guess most users don't know the feature.

I think the specification that if there happens to be a class in App with the same short name,
it will be returned is magical and not CI-like.
I think it is preferable to explicitly set the class to overridden in some way,
so that it is easy to understand and no surprise.

[MGatner]

I'm fine with both of these suggestions. This was developed alongside Myth:Auth where we were going through a whole lot of effort to allow developers to bring their own models. Instead of using a lot of publishing scripts and Config files for model lookup we added the instance App preference. As @iRedds notes this is a sort of stand-in for a service container, which is largely the point of Factories to begin with.

[kenjis]

The entire library makes use of CodeIgniter's Factories to locate the best instances for models. You can supply your own models by duplicating the file names in your app/Models/ folder and applying your customizations.
https://github.com/lonnieezell/myth-auth/blob/develop/docs/extending.md#models

[MGatner]

Yep. Shield does it too, though not sure if we have docs on it explicitly there.

[kenjis]

Shield has User Provider, and there is no docs on Factories.
https://codeigniter4.github.io/shield/customization/#custom-user-provider

[najdanovicivan]

I've noticed this issue as well. The way we can avoid BC is by modifying the way basename works. If just class base name is passed the whole discovery stuff should be triggered so basically it should work the way it works now. But if FQCN is passed the basename should cache by FQCN so that classes with same name from different namespace can be used.

Here is my example where i noticed the issue

use Pages\Models\Page\SitemapModel as PageModel;
use Pages\Models\Category\SitemapModel as CategoryModel;

model(PageModel::class)
model(CategoryModel::class)

With current setup second model() call will return the cached instance of Pages\Modes\Page\SitemapModel which is not good behaviour.

So idea is to cache FQCN if FQCN is passed but if we call model('SitemapModel') then the whole discovery and regular basename caching should work as is.

[kenjis]

So idea is to cache FQCN if FQCN is passed but if we call model('SitemapModel') then the whole discovery and regular basename caching should work as is.

I implemented like that in #7733
Review it, if you have time.

[najdanovicivan]

#7733 is completely fine

The issue with #7733 is that it introduces the BC. I was thinking about implementing quick non BC change for the next patch version so that model() can start behaving properly immediately until there is a merge window for the #7733 in the next minor release.

I'm just not sure if doing that without BC is even possible.

[kenjis]

This bug will be fixed in 4.4.0 by #7733.

@najdanovicivan This behavior has been in place since the beginning and is not urgent enough to be fixed in 4.3.x.
Besides, I hope to release 4.4.0 soon.

[kenjis]

Closed by v4.4.0.