User tests: Successful: Unsuccessful:
Pull Request for Issue # .
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:
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
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
Labels |
Added:
?
|
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?
Does anyone know why Travis throws this error for this PR and how to correct it?
- JControllerFormTest::testConstructor
JDatabaseExceptionConnecting: Could not connect to MySQL.
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.
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?
Start by ensuring JControllerFormTest extends TestCaseDatabase
and we'll go from there.
Category | Libraries | ⇒ | Libraries Unit Tests |
Labels |
Added:
?
|
Great. It worked, also for my other PR #14567 . Thanks Michael
To me, right now, there are some small concern left for this PR:
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
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.
Honestly for something doesn't sound right when we need database based tests when I just want to test the controller.
@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.
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.
@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)
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.
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.
@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
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)
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
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-03-19 08:18:49 |
Closed_By | ⇒ | joomdonation |
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
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)?