lonnieezell / Bonfire

Jumpstart your CodeIgniter web applications with a modular, HMVC-ready, backend.
http://cibonfire.com
1.39k stars 525 forks source link

Use Modules::run() within view and Template::render() #1095

Closed lorenzosanzari closed 9 years ago

lorenzosanzari commented 9 years ago

Hello everyone! Can someone tell me if it was fixed the bug of HMVC (modular extension) when using the method Modules :: run ({module} / {controller} / {method}) inside a view of BF v. 0.7.1-dev? From the discussion forum of BF I understand that the only way to call an action of a module from a view is to use $ this-> load-> view () in the controller that you want to call (you can not use template :: render ()). I modified the code to allow the normal use of the method Template :: render () and everything works, except that in the case where a view you loaded actions belonging to the same context (although of different modules), because in that case would be loaded classes with the same name, then generates an error of class redeclaration. eg. load "content" News from controller module and "content" Pages from the controller module. We have an error for having declared twice the class content, even if the classes belong to different modules. A similar problem should solve using namespaces in PHP, but I would avoid, and opt for changing the names of the controllers (at least those of the application modules), leaving controllers public (frontend) the name equal to the name of the module (BF default naming), and for those of backend choose how to name a structure like "modulename_contextname", so you never have duplicated, even in the same context. To make this scenario work, I have modified the method _load_controller () in bonfire / core / CI_Loader.php (for all controllers loaded via ....-> load-> controller ()) and I realized that I have to change also the statement loading within bonfire / codeigniter / core / CodeIgniter.php (from line 224 onwards, I think). I would like to know though (maybe by Lonnie) if there are other points in the framework in which you run the load controllers, or it may be enough to change only these two parts.

Below is the modified code.

Remember the Modules::run() original code

public static function run($module)
    {
        // Get the arguments to pass them along
        $args = func_get_args();

        // Use the built-in load method to handle this
        return get_instance()->load->controller($module, $args);
    }

Modified _load_controller() method in bonfire/core/Loader.php: add following lines at 614 (before ob_start() ):

Template::set_view("{$class}/{$method}");
Template::$run_mode = true;

Modified Template::render() (inside the controller invoked from view)

public static function render($layout = null) {

        if (self::$run_mode == true) {  ////////////////////// new code ///////////////////
            self::$ci->load->view(self::$current_view, self::$data);
        } else { //////////////////////////////////////////////////////////////////////////////////////////////
            //DEFAULT NATIVE STATEMENT
            // Determine whether to override the current layout.
            $layout = empty($layout) ? self::$layout : $layout;
            // If the current view has not been set, use the current controller/method.
            $controller = self::$ci->router->class;

            if (empty(self::$current_view)) {
                self::$current_view = "{$controller}/" . self::$ci->router->method;
            }
            // Override the layout if this is an AJAX request.
            if (self::$ci->input->is_ajax_request()) {
                $layout = self::$ci->config->item('template.ajax_layout');
                // $controller is passed to load_view to set a controller-based override
                // of the layout, which should not be done for AJAX requests.
                $controller = '';
            }
            // Time to render the layout.
            $output = '';

            self::load_view($layout, self::$data, $controller, true, $output);
            if (empty($output)) {
                show_error("Unable to find theme layout: {$layout}");
            }

            Events::trigger('after_layout_render', $output);
            self::$ci->output->set_output($output);
        }
    }

How to avoid conflict between classes (controllers) with same name (without to use namespaces) ?

Help me, thanks! (...and sorry for my English!) ;-)

lonnieezell commented 9 years ago

I think an easier way of managing what you're trying to do is to re-think the way you use controllers. To be fair, I'm not a fan of loading other controllers from views/controllers in general so I have never gone out of my way to support that, though the HMVC solution we used does.

Here's what I would do:

  1. Think of the controller as only something that takes the request from the user, pulls together the information from other sources (like models and libraries) and send the information back to the user. It should do very little logic.
  2. Remove as much stuff from the controllers out to a library, or the model.

This does a couple of things. First it allows you to clean up your controller. Second, it makes the code in your library much more testable, if that's something you're concerned about. Third, it allows you to load that library from other modules. Finally, this bypasses the need for worrying about class name conflicts since your business logic is encapsulated in the library.

The library method could even return the already rendered HTML, though that's not always ideal, but is fine at times.

The Template::render() method is designed for the sole purpose of rendering out a page, and throwing it inside of the layout. So it's not intended to render views. If you need to render another view from your theme folder, you can use the theme_view() function. (See the end of the Template library for that function).

lorenzosanzari commented 9 years ago

Thanks Lonnie. You are right. It 's true, the controllers should not be called by other controllers or views. But it is useful to have the ability to combine several functions in one action. So how should you do to invoke methods (functionality) of a controller in another controller, without duplicating the code several times? Ex.

MODULE A-> METHOD_A_1 () {

// do something (very very complex code!) ....
.....
Template :: render ();
}
MODULE B-> METHOD_B_1 () {

// do something
.........

// I want here the result of MODULE_A-> METHOD_A_1 ();

....
Template :: render ()

}

Thanks in advance!

lonnieezell commented 9 years ago

That's exactly what Library's are used for.

MODULE A -> LIBRARY FILE() {
    // do something (ver very complex code) ....
}

MODULE A-> METHOD_A_1() {
    $this->load->library('library_file');
    Template::set('something', $this->library_file->method() );
    Template::render();
}

MODULE B-> METHOD_B_1() {
    $this->load->library('module_a/library_file');
    Template::set('something', $this->library_file->method() );
    Template::render();
}

Depending on exactly what the code is doing, it might be better as part of a Model, but the basics are the same. This keeps the code in one place, easily accessible, easily testable, with a public API that can always be kept the same even if the code in the background gets completely refactored into a couple of models, and pulling info from a third party API, or whatever.

mwhitneysdsu commented 9 years ago

I think what it really comes down to is this: Template::render() is meant to be the final call in your application. It is doing a lot more than just loading a view and making data available to that view.

While I can understand the desire to be able to load just about any controller method to generate a partial view, I think it's important to distinguish between partial views and final output. For this reason, the controller methods which I call via Modules::run() are distinct from those which use Template::render(). If two methods within a controller share similar functionality, that functionality is often carried out by a private method in the controller, or by a library or model.

For example, my news controller might do something like this:

class News extends Front_Controller
{
    public function __construct()
    {
        parent::__construct();
        // ...
        $this->load->model('news/news_model');
        $this->lang->load('news');
        // ...
    }

    // Method accessible via route /news or /news/index
    public function index()
    {
        $data = $this->newsFeed();

        // In case this method is modified to use server-side paging
        if (isset($data['pager'])) {
            Template::set('pager', $data['pager']);
        }

        Template::set('records', $data['records']);
        Template::render();
    }

    // Method called via Modules::run("news/partial/{$top}/{$offset}");
    public function partial($top = 0, $offset = 0)
    {
        $data = $this->newsFeed($top, $offset);
        return $this->load->view('news/partial', $data, true);
    }

    private function newsFeed($itemsPerPage = 0, $page = 0)
    {
        $data = array();
        if (! empty($itemsPerPage) && is_numeric($itemsPerPage)) {
            // setup pager
            // ...
            $this->load->library('pagination');
            $this->pagination->initialize($paging);
            $data['pager'] = $this->pagination->create_links();

            if (isset($page) && is_numeric($page) && $page > 1) {
                $this->news_model->limit($itemsPerPage, ($page - 1) * $itemsPerPage);
            } else {
                $this->news_model->limit($itemsPerPage);
            }
        }

        $this->news_model->where($this->news_model->get_deleted_field(), 0)
                                        ->order_by('news_date', 'desc');
        $data['records'] = $this->news_model->find_all();
        return $data;
    }
}

In fact, the above might even have more knowledge of the data than I would normally want in my controller, so I might move all of the news_model code in the newsFeed() method into a single method in the model which can be called in place of find_all().

The point, though, is that there is very little duplicated code between the two methods in the controller, one of which is intended to be called when a user navigates to news or news/index, while the other is intended to return a partial view which displays the same information but is called via Modules::run().

One of the things I've found is that I usually start out with very similar code in the two methods, but over time either the partial or the full view gets modified to handle the data in a manner which is significantly different from the other view. If I had a single method handling both cases, it would eventually just be a big if() statement which performs the duties of two methods without the clarity and separation.

lorenzosanzari commented 9 years ago

Thank you both, and thank to Mat for the valuable example. Everything is clearer to me now. The only question that remains: in the scenario proposed by Mat, also you may have conflicts if from a view are invoked controllers in the same context, because the classes of various controllers will have the same name (same as the name of the context). Is that right?

Eg. (we are in a view): echo Modules::run('news/content/index'); echo Modules::run('clients/content/create');

Both in module News and Clients, controllers (backend controllers) will have 'content' class name, then I'll get redeclaration error. This seems to be a limit of the routing-code_inclusion mechanism, without use namespaces. Is there any solution for this?

mwhitneysdsu commented 9 years ago

Your best bet in these cases would be to use an admin_controller or authenticated_controller which is not in a context (e.g. a public controller instead of a backend controller, but still utilizing authentication, if necessary) or a library.

I would consider this a limitation of both the context mechanism and the CI/HMVC loaders. Namespaced controllers just aren't going to happen completely within the CI/HMVC framework in CI 2 or 3, but you can always load a namespaced controller on your own (or via Composer). You might even be able to take some clues from the MX controller to gain access to CI from within a namespaced controller, if necessary. The existing Modules library and Loader classes are intended to work within CI's framework, though, and the Contexts library just wasn't designed with the idea that you might want to load multiple controllers from the same context at the same time.

lorenzosanzari commented 9 years ago

Everything ok, Mat! I just tested your suggestion with the controller out of context, and everything works perfectly! Thank you so much! ;-)