[4.0] Move addIncludePath() back to trait

[SharkyKZ]

Summary of Changes

Moves the addIncludePath() method back to Joomla\CMS\MVC\Model\LegacyModelLoaderTrait.

Testing Instructions

Install Weblinks https://github.com/joomla-extensions/weblinks/releases/tag/3.7.0
Run this code somewhere:

\JModelLegacy::addIncludePath(JPATH_SITE . '/components/com_weblinks/models');
$model = \JModelLegacy::getInstance('Weblink', 'WeblinksModel', ['ignore_request' => true]);

Check the value of $model.

Expected result

It's an instance of WeblinksModelWeblink.

Documentation Changes Required

No.

[mbabker]

I’d leave things alone as they are now. The issue with a static property on a trait is that it isn’t going to have the same semantics as a static property on a class. See https://3v4l.org/D9KU0 for exactly why being on the trait is not reliable.

[wilsonge]

And I think what your saying was exactly the issue. At a specific point we added paths with BaseModel and then when we retrieved with BaseDatabaseModel (i.e. JModelLegacy) we hit all the issues

[SharkyKZ]

It works either way, as far as I can tell. But maybe I'm missing something.

[mbabker]

If you're calling it as JModelLegacy::addIncludePath() then yes, it'll work 100% of the time, every time.

However, it's a public method. And as we all know, PHP has class inheritance. So you can also call it as JModelList::addIncludePath(), or WeblinksModelWeblink::addIncludePath() from the outside. And inside models, there are undoubtedly static::addIncludePath() calls. This is where it all falls apart.

Review the 3v4l I posted and you'll see why having the property in the trait starts to fall apart at a complete system level (end to end integration, not a specific isolated case where things will work no matter the parent structure). When PHP composes the classes, it essentially inlines the property separately to each class. So FirstStaticPaths::$paths and SecondStaticPaths::$paths are two totally separate arrays. Whereas, a static property in a class is the same in that class and all descendants of that class. So HasPaths::$paths, ThirdStaticPaths::$paths, and FourthStaticPaths::$paths all reference the same array. Because of the static nature of this array, it's the latter behavior that you actually want.

As for a practical demonstration of how this can be an issue, apply this patch on a CMS install and then add this as an onAfterInitialise listener:

public function onAfterInitialise()
{
    \JModelLegacy::addIncludePath(JPATH_ROOT . '/includes');
}

Then, in any request add this snippet inside a model constructor (shouldn't matter the component)

var_dump(static::$paths);

// This should work without an issue because you're inside the model class so the protected property should be in scope and accessible
var_dump(\JModelLegacy::$paths);

// But in the off chance it doesn't, Reflection will let us prove the point
$property = (new \ReflectionClass(\JModelLegacy::class))->getProperty('paths');
$property->setAccessible(true);
var_dump($property->getValue());

The end result is you'll have two arrays with two different sets of paths, the one inside the model class not including the path you added with a plugin at the beginning of the request.