? ? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
12 Mar 2017

Pull Request for Issue # .

Summary of Changes

This PR contains improvement + clean up to controller class to support both namespace and none namespace components in Joomla 4 (backward compatible change). Below are the list of changes/improvements:

  1. Change to base Controller class
  • Allow inject Input data instead of coupling with application input
  • Support creating namespace model/view classes
  • Simplify getName() method (that method was implemented wrong from beginning, instead of returning name of controller, it return component name ,without com_ prefix), just use substr method instead of regex.
  • Implement getControllerName() method to allow getting name of controller properly (as mentioned above, that should be the role of getName() method)
  1. Change to Form Controller class
  • Call getControllerName method (from base controller class) to get context instead of using regex
  • Use toPlural method of \Joomla\String\Inflector class to detect view_list property instead of using a script found elsewhere in the internet.
  • Remove some redundant else if
  1. Change to Admin Controller class

Testing Instructions

  1. Install Joomla 4.0
  2. Apply patch
  3. Try to navigate to different pages (maybe try to add. edit, delete, publish articles), make sure it is still working as before

I also tested with the namespace com_content component in this PR #14370 and it worked well. Hard for end user to test it for now, thought

Need developers to review code and discuss, too

avatar joomdonation joomdonation - open - 12 Mar 2017
avatar joomdonation joomdonation - change - 12 Mar 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 12 Mar 2017
Category Libraries
avatar joomdonation joomdonation - change - 12 Mar 2017
Labels Added: ?
avatar joomdonation
joomdonation - comment - 13 Mar 2017

I have one question here. Is it safe to assume that in context of Joomla Controller, a controller always has input object available? And we can safely get $option directly from Input?

I ask this question is because unit test is failed for this PR. One of the test is validating detect $option from class name

$object = new MincesControllerMince(
	array(
		// Neutralise a JPATH_COMPONENT not defined error.
		'base_path' => JPATH_BASE . '/component/com_foobar'
	)
);

Do we have to implement code to pass this test? Mean when $option is empty from input, we need to detect $option from class name (com_minces in this case)?

avatar joomdonation
joomdonation - comment - 15 Mar 2017

So I changed the code so that the namespace is not getting from manifest cache instead of calculating from class name using reflection. If we are worry about the size of manifest cache, we can introduce a new column to store namespace of extension if needed.

The $prefix parameter or getModel() and getView() method now can has two special value Site/Admin and the controller will create Site or Admin Model/View class. In case it is empty, the current context (depend on application) will be used

@mbabker Could you please take a quick review to see whether it is OK to have it implemented like this?

avatar joomdonation
joomdonation - comment - 16 Mar 2017

Does anyone know why Travis throws this error for this PR and how to correct it?

  1. JControllerFormTest::testConstructor
    JDatabaseExceptionConnecting: Could not connect to MySQL.
avatar mbabker
mbabker - comment - 16 Mar 2017

The test classes only set up a mocked database connection if they extend TestCaseDatabase, or you've explicitly injected a mock database driver into JFactory::$database. So the test class now probably has no database dependencies but your changes are adding one.

avatar joomdonation
joomdonation - comment - 16 Mar 2017

With latest change, I get namespace of component from manifest cache (instead of getting it from class name) as suggested somewhere, maybe that's why database is needed now

Since I don't have any experience with unittest, any instructions to correct this?

avatar mbabker
mbabker - comment - 16 Mar 2017

Start by ensuring JControllerFormTest extends TestCaseDatabase and we'll go from there.

avatar joomla-cms-bot joomla-cms-bot - change - 16 Mar 2017
Category Libraries Libraries Unit Tests
avatar joomdonation joomdonation - change - 16 Mar 2017
Labels Added: ?
avatar joomdonation
joomdonation - comment - 16 Mar 2017

Great. It worked, also for my other PR #14567 . Thanks Michael

To me, right now, there are some small concern left for this PR:

  1. Should we get namespace of extensions from manifest cache as how I am doing in this PR or we should introduce a new field to #__extensions table to store namespace. Manifest cache contains much more data than we need, and we are loading all components into memory, so using Manifest cache is of course expensive than using a separate field

  2. The name of controller classes. At the moment, it is Controller, Form, Admin. Should we change it to different names like BaseController, FormController, AdminController (like how classes are named in some other framework packages). I don't know what's the best practive

Other than that, I think I am done for changing Controller classes to support namespace components. I also opened another PR #14567 for changing Model classes, too and I think I am done for that part, too

Next thing would be the View classes (change it to support namespace, split the View classes to View and Html....to support more format in the future). However, before working on it, I would like to see feedback from the Joomla 4 team to see whether I am on right direction or not. So @mbabker @wilsonge @yvesh @laoneo please help reviewing this PR and #14567 when you have some time.

avatar laoneo
laoneo - comment - 16 Mar 2017

Honestly for something doesn't sound right when we need database based tests when I just want to test the controller.

avatar laoneo
laoneo - comment - 16 Mar 2017

@joomdonation I think it would make sense to have the com_content pr merged first. Like that we can see how it will be used and we can merge your pr's.

avatar joomdonation
joomdonation - comment - 16 Mar 2017

Honestly something doesn't sound right for me when we need database based tests when I just want to test the controller.

I think the Joomla 4 team will need to decide where we get / store namespace of extension first. I remember on different PRs, @wilsonge and @mbabker mentioned that can store it in database (a new field or in manifest cache) to have have a single source to get namespace. That's the reason I made the change.

avatar joomdonation
joomdonation - comment - 16 Mar 2017

@joomdonation I think it would make sense to have the com_content pr merged first. Like that we can see how it will be used and we can merge your pr's.

One thing with merging that PR is it seems it makes cpanel of Joomla4 installation broken (maybe I am wrong), downloaded your repo for testing long time ago and made several changes while testing my PR (I uses your namespaced com_content for testing)

avatar mbabker
mbabker - comment - 16 Mar 2017

Part of the problem we have right now is the MVC layer isn't doing dependency injection of anything it needs, so the base classes themselves are doing service/parameter lookups. We really need to shift the creation behaviors into a proper factory object.

Ya, that means there's still a dependency to an external data provider somewhere, but we're isolating that dependency to a single Joomla\Cms\MVC\Factory type object instead of all over the place.

avatar laoneo
laoneo - comment - 16 Mar 2017

We have been talking about that in London already. I agree that a factory would solve that issue. But I think that switch can be done on a later stage. For me it is more important to go a step further and merge the PR's that we can star thinking about such stuff. IMO we shouldn't do all of the issues we are discussing here in this pr. Let's go step by step.

@joomdonation on some point we need to make that step and for the current state, I don't care if Joomla 4 can be installed trough cPanel. I guess now it is important to move further and not end up in endless discussions.

avatar joomdonation
joomdonation - comment - 16 Mar 2017

@laoneo To be more clear (as I said, maybe my installation was broken because I made some changes to it to be able to test your namespace com_content with my PR), when I login to administrator area http://localhost/namespace/administrator/, I see this error

error_administrator

So if you confirm that you have no error like that, it would be OK.

As we have testers testing others part of Joomla 4, I think we cannot merge PR which result the error like this. If we want to merge the namespace com_content PR, we will have to make sure nothing is broken (maybe there is some modules in backend use com_content models but the none-namespace one and it causes the errors)

avatar laoneo
laoneo - comment - 16 Mar 2017

Yes for sure, it must not be broken ? actually there are also some merge conflicts. But as soon as I have the go from the J4 release leader ( @wilsonge ) then I will solve the merge conflicts.

avatar joomdonation
joomdonation - comment - 16 Mar 2017

Maybe I should still go ahead to work on some changes to View class to support namespace component. Then my PRs even is being merged before your namespace com_content PR, who knows, lol

@mbabker Could you please give more detailed instructions about how should we implement Joomla\Cms\MVC\Factory ? What dependency should be passed to that class? This is honestly, over my skill, thus need more helps

avatar joomdonation joomdonation - change - 19 Mar 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-03-19 08:18:49
Closed_By joomdonation
avatar joomdonation joomdonation - close - 19 Mar 2017

Add a Comment

Login with GitHub to post a comment