? Pending

User tests: Successful: Unsuccessful:

avatar SharkyKZ
SharkyKZ
12 Mar 2020

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.

avatar SharkyKZ SharkyKZ - open - 12 Mar 2020
avatar SharkyKZ SharkyKZ - change - 12 Mar 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 12 Mar 2020
Category Libraries
avatar SharkyKZ SharkyKZ - change - 12 Mar 2020
The description was changed
avatar SharkyKZ SharkyKZ - edited - 12 Mar 2020
avatar SharkyKZ SharkyKZ - change - 12 Mar 2020
The description was changed
avatar SharkyKZ SharkyKZ - edited - 12 Mar 2020
avatar mbabker
mbabker - comment - 12 Mar 2020

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.

avatar wilsonge
wilsonge - comment - 12 Mar 2020

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

avatar wilsonge wilsonge - change - 12 Mar 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-03-12 13:57:06
Closed_By wilsonge
Labels Added: ?
avatar wilsonge wilsonge - close - 12 Mar 2020
avatar SharkyKZ
SharkyKZ - comment - 12 Mar 2020

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

avatar mbabker
mbabker - comment - 12 Mar 2020

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.

Add a Comment

Login with GitHub to post a comment