Skip to content

Conversation

@rdeutz
Copy link
Contributor

@rdeutz rdeutz commented Feb 20, 2020

Pull Request for Issue #24559 .

Summary of Changes

This moves the addIncludeMethod from the LeagcyModelLoaderTrait to the BaseModel and declare the $paths var as static member of the class

Testing Instructions

Check components that are using the addIncludePath method, if they are still working

Expected result

It still works

Documentation Changes Required

none

@richard67
Copy link
Member

The linked issue at the top of the description is wrong. It is #24599 , but it should be #24559 .

@rdeutz
Copy link
Contributor Author

rdeutz commented Feb 20, 2020

Thanks fixed it

@richard67
Copy link
Member

@rdeutz Not really, I still see ‘Pull Request for Issue #24599 .‘.

@rdeutz
Copy link
Contributor Author

rdeutz commented Feb 20, 2020

It's like cs fixes, it always takes two or more attempts to fix a trailing space :-)

@SharkyKZ
Copy link
Contributor

Call to undefined method Joomla\CMS\MVC\Model\BaseModel::addIncludePath() 

Call stack
--
# | Function | Location
1 | () | JROOT\libraries\src\MVC\Controller\BaseController.php:186
2 | Joomla\CMS\MVC\Controller\BaseController::addModelPath() | JROOT\libraries\src\MVC\Controller\BaseController.php:460
3 | Joomla\CMS\MVC\Controller\BaseController->__construct() | JROOT\libraries\src\MVC\Factory\MVCFactory.php:77
4 | Joomla\CMS\MVC\Factory\MVCFactory->createController() | JROOT\libraries\src\Dispatcher\ComponentDispatcher.php:172
5 | Joomla\CMS\Dispatcher\ComponentDispatcher->getController() | JROOT\libraries\src\Dispatcher\ComponentDispatcher.php:145
6 | Joomla\CMS\Dispatcher\ComponentDispatcher->dispatch() | JROOT\libraries\src\Component\ComponentHelper.php:384
7 | Joomla\CMS\Component\ComponentHelper::renderComponent() | JROOT\libraries\src\Application\AdministratorApplication.php:115
8 | Joomla\CMS\Application\AdministratorApplication->dispatch() | JROOT\libraries\src\Application\AdministratorApplication.php:158
9 | Joomla\CMS\Application\AdministratorApplication->doExecute() | JROOT\libraries\src\Application\CMSApplication.php:230
10 | Joomla\CMS\Application\CMSApplication->execute() | JROOT\administrator\includes\app.php:63
11 | require_once() | JROOT\administrator\index.php:36

@SharkyKZ
Copy link
Contributor

Last commit reintroduces the original issue.

@SharkyKZ
Copy link
Contributor

This works but if we go this way the method should stay in LeagcyModelLoaderTrait (according to #24559).

@jwaisner
Copy link
Member

@rdeutz Can you clarify in your testing instructions what components are affected by the change? I want to ensure that this is tested properly.

@chmst
Copy link
Contributor

chmst commented Mar 3, 2020

I have tested this with differenc components: com_cache, com_users, com_content and looks good.
But recommend more tests, especially with 3rd party components


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/27998.

@jwaisner
Copy link
Member

jwaisner commented Mar 6, 2020

@rdeutz Can you please resolve conflicts?

@wilsonge
Copy link
Contributor

wilsonge commented Mar 9, 2020

LeagcyModelLoaderTrait just realised this has a typo in xD

@wilsonge
Copy link
Contributor

Confirmed with the weblinks module that this fixes the issue. Thankyou!

@wilsonge wilsonge merged commit 759c5d9 into joomla:4.0-dev Mar 11, 2020
@wilsonge wilsonge added this to the Joomla 4.0 milestone Mar 11, 2020
@SharkyKZ
Copy link
Contributor

Wasn't this method supposed to stay in the trait?

@wilsonge
Copy link
Contributor

Ideal world yes. But because of php internals it doesn’t work. See the original issue

@SharkyKZ
Copy link
Contributor

It works either way. The fix was moving caching to class property. PR for moving back the method #28322.

@rdeutz rdeutz deleted the pr_issue_24559 branch February 8, 2021 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants