fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

loading Module->index ok, loading other actions -> 404 #82

Closed micwehrle closed 13 years ago

micwehrle commented 13 years ago

Quick caveat emptor: my first report of that kind, so I am not even sure this is the right place......

unless I am completely wrong, there is an error in the Router::find_controller method

My setup:

modules ---test ------controller ---------test.php

test.php:

<?php
namespace Test;

class Controller_Test extends \Controller {

    public function action_index()
    {       
        $this->response->body = 'abc';
    }

    public function action_login()
    {
        $this->response->body = 'def';
    }

}
/* End of file test.php */

\ what fixed it **

/core/classes/router.php (around line 135):

was:

$match->controller = count($segments) ? array_shift($segments) : $match->module;
``

fix:
```php
$match->controller = count($segments)>1 ? array_shift($segments) : $match->module;
crynobone commented 13 years ago

Is your test.php file location at /modules/test/classes/controller/test.php or /modules/test/controller/test.php?

micwehrle commented 13 years ago

The former:

modules/test/classes/controller/test.php

micwehrle commented 13 years ago

Upon further development/testing found that additional segments (e.g.: http://myserver/test/view/2) will also lead to a 404 (of course by that time an action_view method exists)

back to core/router.php ~line 145: an additional iteration of segment parsing seems to help here.

// if we know and even the above fails, then I think we can assume the following:
                        $segments = $match->segments;

                        $match->module = array_shift($segments);
            $match->directory = null;
                        $match->controller = $match->module;

                        if (class_exists(ucfirst($match->module).'\\Controller_'.ucfirst($match->controller)))
            {
                $match->action = count($segments) ? array_shift($segments) : 'index';
                $match->method_params = $segments;                        
                return $match;
            }
crynobone commented 13 years ago

Shouldn't you be accessing it from http://myserver/test/test/view/2

http:// [server] / [module] / [controller] / [method]

micwehrle commented 13 years ago

I know that would work, but .... I don't know if that is the intended way... maybe I'm confused about models and how to call them, but the looks of the URL will be gone.... What am I getting wrong?

micwehrle commented 13 years ago

... I meant "modules"

ralf57 commented 13 years ago

I have experienced the same "problem". Provided that you have a "forum" module, the expected behaviour would be:

http://myserver/forum/view/2

and not

http://myserver/forum/forum/view/2

@micwehrle feel free to submit a pull request with your fix so it can be reviewed by the developers

ralf

micwehrle commented 13 years ago

ralf, would love to.... just no idea how to yet....

ralf57 commented 13 years ago

it's not that hard: http://help.github.com/pull-requests/

cheers

crynobone commented 13 years ago

I don't see it a good idea to use that sort of solution, how ever you should know that you overwrite the default routing http://fuelphp.com/docs/general/routing.html

WanWizard commented 13 years ago

This is something that doesn't need fixing in code imho. You should use the framework as it's supposed to be used, not modify the code because you use it the wrong way.

Without routing, http://server/forum/view/2 refers either to module 'forum', controller 'view', or app controller 'forum'. Since you're already in a module called 'forum', I think it's pointless creating a controller called 'forum' in that module. Create a controller called 'view' instead.

micwehrle commented 13 years ago

WanWizard: I hope I am not beating a dead horse... but creating controllers doing CRUD methods..... that doesn't seem right either.

WanWizard commented 13 years ago

I miss the relevance of that remark to this issue?

micwehrle commented 13 years ago

sorry. You mentioned creating a controller "view" instead of a controller "forum" in that module. In my case "view" is a method in the controller "forum" (one of the CRUD methods, the other methods in the controller "forum" might be "create", "delete", etc.). Interpreting your suggestion to me means creating a controller "view" that has one method "index" that would create the view , then creating a controller "delete" that would handle the deletion of the forum....etc.

WanWizard commented 13 years ago

I actually do that. The simpler the controller, the better.

It's either that, or use a route to get the redundant forum out of the URI. The only other option would be to add logic to the router to check for a controller with the same name as the module if the 'detected' controller isn't found.

But as that means extra code, extra checks, it will slow things down, and it's not something I would like to do.