Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix module reserved routes #2013

Open
wants to merge 1 commit into
base: 1.9/develop
Choose a base branch
from

Conversation

nnssn
Copy link
Contributor

@nnssn nnssn commented Aug 5, 2016

Routing "400" which is defined in the module has been fixed a problem that is not called. Consider the possibility that in the future new exception is added, it was to handle the 400 series to 500 series as a reserved route.

example: fuel/modules/api/config/routes.php
return array(
    '_400_'   => 'api/controller/400',
    '_403_'   => 'api/controller/403',
    '_500_'   => 'api/controller/500',
);
result
api/_400_  // not called
_403_
_500_

In addition, it seems "Module :: unload" at 400, 403, 500 has not been properly handled. The problem I think most impact was not.

@WanWizard
Copy link
Member

It was never in the design that a module route could overload a global route. For that reason, all module routes have to be prefixed with the module name for them to be accepted. Which obviously doesn't happen for these "system" routes.

You see that "root" is translated to the module root, and not the app root. All other system routes should be rejected, they are not allowed in a module.

Problem is that you might get away with it when you use normal URI routing (because the module routes of the module you are routing to are always loaded last), but it gets messy if you autoload modules, manually load modules, and use HMVC calls to module controllers so you have multiple requests active. Which 404 route is active at which point?

To solve this properly, you need a route stack, which requires a complete redesign of the routing mechanism, which I think should not be in the scope of version 1 anymore.

@WanWizard
Copy link
Member

@stevewest your two cents on this?

@emlynwest
Copy link
Contributor

I agree that this would be a very useful feature to have, your api 404 is not going to be the same as your "user facing" 404 after all. But I do agree that this change is not in the scope of v1 due to the large change that would be needed in the framework internals to accommodate this.

@nnssn
Copy link
Contributor Author

nnssn commented Aug 5, 2016

Users, administrators, each has defined its own 404route to both modules. I tried to call a non-existent function of the users module in HMVC request from the administrator module.

Expected without my evidence before the execution was that 404 of the administrator is executed. But, it was actually executed was the 404 of the last loaded users module. I felt that it is a natural result from getting after. This is because an exception has occurred is the users module.

Even the call of simple single-function I felt that there is a need to pay attention. I did not know where to as a reference. If the cause of all to have a consistency, it seems to involve a major change as you are told.

@WanWizard
Copy link
Member

WanWizard commented Aug 7, 2016

I think the easiest to implement is to split the functionality:

  • Use Module::load() and Module::unload() to process all module route not starting with "_".
  • Have request call Module::load() when a module is detected, instead of parsing routes itself
  • Have Request::execute() swap the global "_" routes for module ones if needed, and swap them back when the request is finished, like it is done with the language now.

I'll try to find some time this week to look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants