HMVC with Core Unit Tests #1818

Closed
wants to merge 74 commits into
from

Conversation

Projects
None yet
Contributor

dchill42 commented Sep 21, 2012

This is the HMVC solution from #357, further refined and with thorough unit tests added.

The former incarnation was recommended by @philsturgeon, who suggested several features and refinements along the way. All of the substantive changes - especially the CodeIgniter class and modifications to Config, Router, and Loader - have been stable and working in a production environment for over a year and a half now.

Almost all core classes have unit test coverage, and specifically those with new features or significant changes are all covered, with strong isolation and virtually every code path tested. Convenience functions for vfsStream have also been added to CI_TestCase.

A new README in the system directory enumerates the entire application lifecycle in detail to aid in comparison with the existing bootstrap process.

This implementation aims to be fully backward compatible with version 2.1. Great care has been taken to ensure that upgrading users can continue to use the framework in the same way they used to, even if they don't take advantage of the HMVC feature.

P.S. - Don't be frightened by the number of new lines of code in this request - 2/3 of that is new unit test code.

Good fix! Supporting under 5.3 is a pain but one of the biggest draws for people to use this framework. i.e: portable as f**k.

Owner

dchill42 replied Sep 1, 2011

Thanks! I'm happy to support older versions of PHP5 - I just missed that array_replace_recursive() was 5.3+. I just uploaded the last tweak (which I missed in this commit), that condenses the code inside the _merge_arrays() loop with a ternary operator.

dchill42 and others added some commits Sep 1, 2011

Added VFS helpers to CI_TestCase and finished Config unit
Signed-off-by: dchill42 <dchill42@gmail.com>
Loader unit test improvements
Signed-off-by: dchill42 <dchill42@gmail.com>
Added many more Loader unit tests
Signed-off-by: dchill42 <dchill42@gmail.com>
Added Router unit test
Signed-off-by: dchill42 <dchill42@gmail.com>
Merge branch 'develop' of github.com:/EllisLab/CodeIgniter/ into hmvc…
…_unit and Cleanup

Signed-off-by: dchill42 <dchill42@gmail.com>
Post-merge file restore
Signed-off-by: dchill42 <dchill42@gmail.com>
Added Controller and Log unit tests and style cleanup
Signed-off-by: dchill42 <dchill42@gmail.com>
Added Output unit test, Config return value feature, and bugfixes
Signed-off-by: dchill42 <dchill42@gmail.com>
Minor tweaks
Signed-off-by: dchill42 <dchill42@gmail.com>
Contributor

dchill42 commented Sep 21, 2012

@alexbilbie and @narfbg - Please take a serious look at this submission. @philsturgeon had wanted to merge it in for a long time, but we kept running into roadblocks. Also bear in mind that HMVC has already been announced as a planned feature for 3.0.

I would really like to share this functionality with the entire CI community, and I will gladly make further revisions to it in order to aid its acceptance. Documentation and Changelog updates are still due to be added - I will write those up if this reaches a point of serious consideration.

I'm also happy to add code comments to the request if that helps navigate the sea of changes and explain the reasons for them (or for things like where certain blocks of code moved to).

Contributor

daparky commented Sep 21, 2012

I would be really to happy to see this merged in, CI needs HMVC out of the box.

Contributor

tkaw220 commented Sep 21, 2012

+1

Contributor

alexbilbie commented Sep 21, 2012

@dchill42 any chance you could pull in the latest CI develop branch so that I can have a proper play with this please?

Contributor

dchill42 commented Sep 21, 2012

@alexbilbie Pagination change pulled in - everything else was already up to date.

Contributor

alexbilbie commented Sep 21, 2012

Ta, I'm going to have a play today. Have you documented how to use it in the docs?

Contributor

dchill42 commented Sep 21, 2012

Not yet. The key is that you can call CI_Loader::controller() with a URI-style route to load and run another controller. Example:

$this->load->controller('main/handler/arg1/arg2');

As you can see, arguments may be included in the URI. Routes do apply. Optional object name may be passed as a second parameter (e.g. - to load as $this->foo). Optional third parameter may be set to FALSE to refrain from calling method yet (just loads controller object), or to TRUE to run controller and return method return value instead of success/failure flag.

A variant form is CI_Loader::controller_output(), which takes a reference to a string as the first parameter for capturing output, followed by all the same parameters as the first form.

The original routed controller is attached by its name and also by the special handle "routed", so it can always be identified.

There are lots of other little features and improvements (like error/404 route overrides pointing to controllers), but that's the big change. As long as this request isn't going to just be shelved or back-burnered, I'll be happy to write up the full doc changes and submit them. I just didn't want to do all the work for nothing - writing up all those unit tests was enough of a time commitment without any assurances.

Contributor

dchill42 commented Sep 21, 2012

Do take a look at system/README.md - that gives a decent overview of the key features in a doc sort of way. It's a bit of a stopgap to updating the real help files.

dchill42 added some commits Sep 21, 2012

Make Travis happy
Signed-off-by: dchill42 <dchill42@gmail.com>
Try again - still had errors
Signed-off-by: dchill42 <dchill42@gmail.com>

dchill42 added some commits Sep 24, 2012

Make Uploader test work on local machine and Travis
Signed-off-by: dchill42 <dchill42@gmail.com>
Make ENV config override standard config as in #1536
Signed-off-by: dchill42 <dchill42@gmail.com>
Added exception_override routing for exception handler
Signed-off-by: dchill42 <dchill42@gmail.com>
Owner

dchill42 commented on c0dd048 Sep 24, 2012

Er, #1356 that is...

Contributor

GDmac commented Oct 14, 2012

Can you update the demo to the newest hmvc_unit?
e.g. for loading specific models/libraries $this->CI->load->model('spot/light');

In the current demo i duplicated the page module to 'test' and altered the menu-model with some bogus menu-items for testing purposes. Even when adding the package path in the demo controller for the test module 'after' the add_path for the page module, the menu-model is loaded from the test module.

  1. should (or can) the new module_path also be used inside the page controller to load a model? e.g. load(page/menu) (or actually, to make sure it loads its own model, not the first one found.)
  2. i expect (especially future third party modules) to potentially have identical libraries and/or models. I suspect that with class declaration collisions only one of those can be loaded, yes/no?
Contributor

dchill42 commented Oct 16, 2012

As soon as I come back around to this branch, yes - I will update the demo a bit. As far as I've been told, HMVC has been pushed back to a later release than 3.0, and I have some other unit test improvements I'd like to get into the next release.

  1. Module paths are applied to all MVC objects, but only after the regular package paths have been searched. This means that an identically named object in the package path will supersede one in the module path. I believe I covered that in the user guide updates here, but if it's not clear enough let me know.
  2. I'm not sure how much name collision to expect in any given application. It's not a problem I've personally encountered much, but bigger apps with lots of third-party add-ons might run into this. But here's the catch: all CI object filenames are related to the class names they contain. You can only define a class once in a PHP request, so if you have name conflicts you have to change the name of the class (and the file) to use them simultaneously. The only resolution to that I can think of is to use actual PHP namespaces to separate them, which would also require editing the classes, and probably changes to CI as well. There isn't a simple band-aid for that problem.
Contributor

GDmac commented Oct 16, 2012

Add_package_path() uses array_unshift, so CI will look first in the path that has lastest been added.
(i have tested this in the current hmvc-demo)

If 'modules' is to become a generic/default search-path, am i correct to asume that CI will then load libraries/models form any subdir in the 'modules' directory, after ! any added package_paths?

Contributor

dchill42 commented Oct 16, 2012

Yes - package paths are searched LIFO-style, with APPPATH at the end of the list. When loading Modules, Views, and Controllers in HMVC (as it stands now), if a match is not found in those paths, then any configured module paths are checked - in the order configured.

I would be interested to see any discussion of whether the module paths should be checked first. That would be easy to do, but when I added that feature, I didn't have much to go on as far as priority of paths. My thoughts were that in a single project, a developer is probably either using package paths or the module paths, but not both. In that case, if the object isn't under a subdirectory in APPPATH, the module paths get checked next. That approach rules out the simple case first before running the more complex code. (APPPATH only supports one subdirectory - module paths can be recursive.)

Contributor

GDmac commented Oct 16, 2012

personally i like the idea of having to specify the module from which to load stuff
(e.g. libraries, controllers and what-not). Like as you mentioned way above, in this thread:

$this->CI->load->controller('spot/light');
// or maybe
$this->CI->load->library('spot/batteries');

not just go trawling thru the modules directories.

Contributor

dchill42 commented Oct 16, 2012

Yes, that's the way that works, although modules doesn't cover libraries at this point. However, you can configure more than one modules directory if you want, hence searching the modules path.

If you just use the default 'modules', without any package paths added, it would check APPPATH/controllers/spot and then APPPATH/modules/spot/controllers for the light.php file.

FDiskas commented Oct 24, 2012

I love your idea, but I'm getting a error

404 Page Not Found
The page welcome/index you requested was not found.

I do not modified any files - just downloaded your GIT version

+ $this->benchmark->mark('controller_execution_time_( '.$class.' / '.$method.' )_start');
+
+ // Load the controller, but don't call the method yet
+ if ($this->load->controller($route, '', FALSE) == FALSE)
@FDiskas

FDiskas Oct 24, 2012

I think that there must be
if ($this->load->controller($route, '', FALSE) === FALSE)

@FDiskas

FDiskas Oct 24, 2012

This fixed my 404 error

Contributor

dchill42 commented Oct 24, 2012

@FDiskas thanks for the feedback - I'll check on that error as soon as I'm back to working on this again. There are a few other changes that need to be made before this gets merged, but I hope you'll find that it generally works as advertised.

FDiskas commented Oct 24, 2012

Yes, thanks for the great work. I was searching for HMVC implementation, but most popular modifications was not working as it should. At least I found your pull request. Thanks again. This works as I want.

Still missing some implementation for benchmark - I'm requesting demo/index
public function index()
{
$this->load->view('demo');
$this->load->controller('welcome');
}

BENCHMARKS
Loading Time: Base Classes 0.0139
Controller Execution Time ( Index / Index ) 0.0027
Total Execution Time 0.0169

Results

I enabled $config['log_threshold'] = 4;
And I'm getting browser error "The connection was reset"
My PHP Version 5.3.9

FDiskas commented Oct 25, 2012

@narfbg what do you think? Is this pull request will be accepted and when? We want to start a big project and we are waiting for official HMVC support.

FDiskas commented Oct 29, 2012

I got apache fatal error
The browser shows a message "The connection was reset"
Under PHP Version 5.3.9
I was using http://www.usbwebserver.net/en/ v8.5
I tried to debug every line of code and got an strange results on line 272 in file system/core/CodeIgniter.php

https://github.com/dchill42/CodeIgniter/blob/59f7d20b74c0424f15b57af690db735c6b2e470d/system/core/CodeIgniter.php#L272

After the line:

$filenm = 'core/'.$pre.$class.'.php';

I added

print_r($packages);

And the result was endless loop of

...
Array
(
    [0] => D:\***\web\usb\root\cih\application/
)
Array
(
    [0] => D:\***\web\usb\root\cih\application/
)
...

How can I fix that?
Could it be a problem with a line in index.php file?:

CodeIgniter::instance()->run();

I updated the PHP Version to 5.4.8
the problem still exists.

Apache Version Apache/2.2.21 (Win32) PHP/5.4.8
In apache error logs I see a line:

[Mon Oct 29 22:21:19 2012] [notice] Parent: child process exited with status 3221225725 -- Restarting.

FDiskas commented Oct 29, 2012

looks like that it's trying to load
self:: = new CodeIgniter
class too many times.

self::$instance = new $class($config, $autoload, $basepath, $apppath);

To reproduse that error in config.php file enable log_threshold

$config['log_threshold'] = 4;

and add

echo '<pre>';
debug_print_backtrace();
echo '</pre>';

before the line

self::$instance = new $class($config, $autoload, $basepath, $apppath);

in CodeIgniter.php file

FDiskas commented Oct 30, 2012

Yes, I found a bug.
https://github.com/dchill42/CodeIgniter/blob/59f7d20b74c0424f15b57af690db735c6b2e470d/system/core/Common.php#L95
There is used a constants witch is not loaded here and there was an error witch is handled by
https://github.com/dchill42/CodeIgniter/blob/59f7d20b74c0424f15b57af690db735c6b2e470d/system/core/CodeIgniter.php#L780
and then there was an endless recursive to function
https://github.com/dchill42/CodeIgniter/blob/59f7d20b74c0424f15b57af690db735c6b2e470d/system/core/CodeIgniter.php#L798
a quick fix is just replace a function
https://github.com/dchill42/CodeIgniter/blob/59f7d20b74c0424f15b57af690db735c6b2e470d/system/core/Common.php#L81

    function is_really_writable($file)
    {
        // If we're on a Unix server with safe_mode off we call is_writable
        return is_writable($file);
    }

FDiskas commented Nov 2, 2012

Any official updates?

Contributor

dchill42 commented Nov 2, 2012

@FDiskas I haven't had a chance to really look into this yet. I can tell you, however, that the FOPEN_WRITE_CREATE constant referenced on line 95 of Common.php is defined in the application/config/constants.php file. If your application has removed this file or its contents, that will break CI.

As for the recursive calls to instance() before self::$instance is set, that is a scenario against which we may need to install greater protection. I will investigate further when I return to working on this pull request in the near future.

Thanks for your testing and feedback, and I hope the information about that constant helps you fix your app.

FDiskas commented Nov 6, 2012

Please review the Loader.php
there is some problem then I trying to load a view in modules directory within a directory

modules/services/views/additional/info.php
$this->load->view( 'services/additional/info' );

This is impossible to load a view in a subdirectory in module
To fix that I modified a Loader.php

===================================================================
--- system/core/Loader.php  (revision )
+++ system/core/Loader.php  (revision )
@@ -1084,14 +1084,14 @@
                    unset($_ci_slash);

                    // Search module paths for view
-                   $_ci_viewpath = $_ci_subdir.'views/'.$_ci_file;
                    foreach ($this->_ci_module_paths as $_ci_mod_path)
                    {
                        // Does the view exist in the module?
-                       if (file_exists($_ci_mod_path.$_ci_viewpath))
+                       $_ci_subdir = preg_replace('/([^\/]+)/', '$1/views', $_ci_subdir, 1);
+                       if (file_exists($_ci_mod_path.$_ci_subdir.$_ci_file))
                        {
                            // Set path, mark existing, and quit
-                           $_ci_path = $_ci_mod_path.$_ci_viewpath;
+                           $_ci_path = $_ci_mod_path.$_ci_subdir.$_ci_file;
                            $_ci_exists = TRUE;
                            break;
                        }

Now its possible to load a view file in a subdirectory in module views directory

FDiskas commented Nov 8, 2012

Fatal error in: system/libraries/Form_validation.php 405
Method with a same name already defined in this class: error_array()
I just removed that duplicate method

FDiskas commented Nov 11, 2012

Undefined variable: basepath
Filename: database/DB_driver.php
Line Number: 1366

I replaced to BASEPATH from $basepath


Undefined index: file
Filename: database/DB_driver.php
Line Number: 1363

FDiskas commented Nov 12, 2012

Some bug in system/libraries/Parser.php
I can't parse a string

$string = $this->load->view( 'demo_module/demo_view', $aData, true );
$this->parser->parse_string( $string, $aData );

I got an error:

A PHP Error was encountered
Severity: Notice
Message: Trying to get property of non-object
Filename: libraries/Parser.php
Line Number: 126

Missing $this->CI ??

fixed: EllisLab#1992

FDiskas commented Nov 16, 2012

go baby go. Please update your code

FDiskas commented Nov 27, 2012

There is a bug in Loader.php Then you try to extend the Controller with MY_Controller got an fatal error

Fatal error: Class 'CI_Controller' not found in application/core/MY_Controller.php on line 13

First need to load the core and then my controllers. Just move include_once($base); over include_once($path);

--- <html>Loader.php (<b>283e63b</b>)</html>
+++ <html><b>Current File</b></html>
@@ -983,8 +983,8 @@

     // Include extension followed by base, so extension overrides base functions
     // If this is for a base class, the order won't matter
-    include_once($path);
     include_once($base);
+    include_once($path);
\ No newline at end of file
     return TRUE;
    }
   }

FDiskas commented Nov 27, 2012

There is an bug - it's impossible to load two modules with a same class name and method

$this->load->controller('user_module/index/test');
$this->load->controller('file_module/index/test');

In this case the first class method is executed twice.

Contributor

GDmac commented Nov 27, 2012

In general in php, without namespaces, it is impossible to declare the same classname twice.

FDiskas commented Nov 27, 2012

HMVC will make no sense if the error will not be corrected. Probably the only way is to use "namespace"

Contributor

GDmac commented Nov 27, 2012

CI 3.0 is not going to use namespaces if i recall.
Why would you want to have a duplicate controller (or model) classname anyway?
Two "blog" controllers? two "user_model" models?

For instance, i have a small user_basic model for login and other quick read only stuff like full_name etc..
the user_extended model is more extended (pun intended) and has write methods too and other logic (presenters, validation, etc.). It's only loaded when needed (admin tasks), that could be an approach.

FDiskas commented Nov 27, 2012

In HMVC folder structure

[controllers]
   - profile.php
[modules]
   - [user]
      - [controllers]
         - index.php
   - [blogs]
      - [controllers]
         - index.php

I have profile.php controller
To load user profile and user blog post I must to load a "user" and "blogs" module with a same "profile" method.

$this->load->controller('user/index/profile');
$this->load->controller('blogs/index/profile');

where "user/index/profile"
"user" is module dir
"index" is class name
"profile" is method

Imagine that I have super big project - do I need to care about naming in modules methods?

FDiskas commented Dec 4, 2012

I'm working on a large project using this pull request. No more problems I have noticed so far.
My main concern is - if HMVC ever going to be officially supported. Could anyone approve this?

I pulled this commit about 2 weeks ago and started to throw everything at it I could think of. Not unit test per-say but actual controllers and code. Things like Does it work?, Does it do what I want, Do the standard CI tricks still work work? Does it break anything? So far it looks pretty solid.

Like the comment above will this be official at some point? Of course I will quickly become out of date if I actually started to use this but, I really like it.

BTW made FDiskas change above to fix one CI trick on MY_Controller extending (loader.php)

Contributor

narfbg commented Dec 17, 2012

I'm pretty sure I've already answered this in the comments - there are plans for official HMVC support, but it's not going to be in CodeIgniter 3.0.

FDiskas commented Jan 1, 2013

Missing something like

$this->router->fetch_module();

FDiskas commented Jan 9, 2013

I got an notice.

$this->load->helper('file');
echo get_mime_by_extension('test.html');
A PHP Error was encountered
Severity: Notice
Message: Only variable references should be returned by reference
Filename: core/Common.php
Line Number: 214

I Fixed that by replacing the function from CI development

@FDiskas FDiskas referenced this pull request in linuxjuggler/codeigniter-amazon-sdk Jan 31, 2013

Closed

Failed to determine HOME directory after trying "cd: 1: can't cd to ~" #5

FDiskas commented Mar 10, 2013

Looks like everything is working good. Missing some features like configuration folder in modules dir
like:

application
   modules
      demo_module
         config
            config.php
            routes.php
         controllers
         views

The main problem is the routes.php

FDiskas commented Mar 27, 2013

Only variables should be assigned by reference system\libraries\Upload.php 82

FDiskas commented Apr 14, 2013

I love this pull request. Already done two projects with this. Thanks again.

gurre commented May 10, 2013

Much needed!

Contributor

lonnieezell commented Nov 13, 2013

I'm assuming that a year hasn't been long enough to get the testing done properly on this feature and get it merged in core? So it's still not slated for a 3.0 release? And with EL trying to find a new owner we shouldn't expect any movement on this until sometime after the new owner gets it, and possibly not even then?

Tragedy. This branch is a very nice implementation into the core of something that many, many developers outside of EL would love to have as part of core.

Contributor

dchill42 commented Dec 23, 2013

Sorry for being unresponsive here for a while. Thanks to those who have commented and complimented and offered bugfixes and such. I would still like to see this get merged, but after several cycles of having hope and being put off, I stopped spending much time on it. Given the current status of CI's future, I don't have much hope of any movement soon, either. Whenever the presiding powers (old or new) get serious about HMVC, I'll bring this pull up to date (or create a new, cleaner one) so it can be adopted.

Thanks for your hard work either way. I to have moved onto other things...

FDiskas commented Feb 23, 2014

Please help. Then there is an DB error for example "no table" the FATAL error appears.

Fatal error: Call to a member function show_error() on a non-object in /var/www/system/database/DB_driver.php on line 1197

I have no idea how to fix that. :(

Why there must be 4 segments? So how we load controller from other controller?
screenshot: http://yadi.sk/d/Lwo_IXFCJfppf

@FDiskas FDiskas referenced this pull request in jamierumbelow/codeigniter-base-model Jun 27, 2014

Open

MVC architecture not working #112

FDiskas commented Jul 7, 2014

Migration is not working with PDO driver. Especially down methods.

If you were to make this hmvc with so you can call controller in to controller and echo as string makes simple life rather than echo modules run on to the view. like be able to do this $data['header] = $this->load->controller('admin/common/header/index); on view would be

Contributor

ivantcholakov commented Oct 20, 2014

@riwakawebsitedesigns

The goal of HMVC is adding visual widgets without touching the initial controller, controllers have to be isolated. The intention here is correct. Otherwise, what you've proposed should not be labeled as "HMVC".

FDiskas commented Nov 26, 2014

Imposible to load controller within an directory

--- system/core/Loader.php  (date 1417005333000)
+++ system/core/Loader.php  (revision )
@@ -361,7 +361,7 @@

            // Include source and instantiate object
            // The Router is responsible for providing a valid path in the route stack
-           include($path.'controllers/'.$subdir.strtolower($class).'.php');
+           include($path.'controllers/'.(!empty($subdir)?$subdir.'/':'').strtolower($class).'.php');
            $classnm = ucfirst($class);
            $this->CI->$name = new $classnm();
+
+ // Include source and instantiate object
+ // The Router is responsible for providing a valid path in the route stack
+ include($path.'controllers/'.$subdir.strtolower($class).'.php');
@FDiskas

FDiskas Nov 26, 2014

Must be

include($path.'controllers/'.(!empty($subdir)?$subdir.'/':'').strtolower($class).'.php');
Contributor

ivantcholakov commented Jan 15, 2015

@FDiskas What is this on the picture, a Java application?

Edit 16-JAN-2015:

Implementations may vary, yes.

Let us suppose that I am wrong. I can accept wording "HMVC" about controller loading controller.

But, for a PHP application what is the sense the parent controller to call a child controller, to take the generated HTML and then to pass it within its own view? Memory consumption is the smallest problem.

Let the designer say "I don't need this widget" and remove it from the main view. But the child controller that serves the widget will be still active within the main controller. You will have to say that designer should clean the controller too and to turn him/her into a programmer.

If your child widget is independent, then the designer will be free to add it wherever he/she wants within the view without risk for breaking the main controller code. Separation of concerns is good.

Another possible case: Let us say you have several children widgets. You call their controllers, take HTML and then you make several sort of $this->load->view('whatever', array('html' => $widget_result_html_x)); within the main controller. No. It is the designer to decide the visual order of the widgets.

There may be good exceptions, but generally, calling controller within a controller for displaying HTML is a bad idea.

Contributor

narfbg commented Jan 20, 2015

Closing this ... even if we get HMVC in CI4, it will be on top of completely different code.

@narfbg narfbg closed this Jan 20, 2015

@narfbg narfbg removed this from the v.4.0.x milestone Jul 17, 2015

Great 👍

FDiskas commented Apr 17, 2017

Great - on CI4 there is still no HMWC :) :shipit:

Contributor

lonnieezell commented Apr 17, 2017

@FDiskas Not by name, but pretty damn close, anyway:

https://bcit-ci.github.io/CodeIgniter4/general/modules.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment