fuel / core

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

v1.8.2 caused package/config/routes.php to stop registering with the router automatically #2142

Open iturgeon opened 4 years ago

iturgeon commented 4 years ago

We've been lagging behind in updating, so I apologize for the tardiness. The following commit https://github.com/fuel/core/commit/7fc62869b64a055e1887bd6e31bd0eee9b85a008 causes any packages that were previously using a config/routes.php to create routes to break.

Reading the docs, perhaps this wasn't an intended feature in the first place?

I'm mostly making a comment here to make the maintainers aware of the change and leave a trail for others that may have the same issue.

Our current workaround is to use the package's bootstrap.php file to load the config and manually add it's routes to the Fuel Router:

// Load and register routes for this package
$package_routes = \Config::load(__DIR__.'/config/routes');
\Router::add($package_routes);

This does change the order that the routes are registered (package routes came after app routes before), which may which pattern matches first in the router?

WanWizard commented 4 years ago

Packages are backend code, they should not contain routes?

iturgeon commented 4 years ago

Fair, totally understand if this doesn't deserve attention.

It's been a while... either the docs matured or, more likely, my understanding of Fuel was just incomplete at the time. If anyone else out there painted outside the lines and your package's routes break - the workaround above is a cheap fix, otherwise convert it to a module.

WanWizard commented 4 years ago

Any idea what exactly broke it? And what it the definition of "broken"?

iturgeon commented 4 years ago

The package in use here was to add onelogin/php-saml capability to our project.

The package's routes were anonymous functions that executed an adapter class method.

Previously fuelphp would automatically load the config/routes.php from the package routes and add them to the application.

The change caused those routes to no longer get registered with the router. Hence breaking our SAML integration.

WanWizard commented 4 years ago

Related to https://github.com/fuel/core/commit/7fc62869b64a055e1887bd6e31bd0eee9b85a008#diff-abf8fc3534cc67ebd9af340c3e9b36d0 ?

iturgeon commented 4 years ago

Correct, previous version was 1.8.1.6. I applied just these changes to fuel.php and the routes in a the package stopped automatically loading.

WanWizard commented 4 years ago

Ok. Understandable, this moves loading the routes to before packages are autoloaded.

glOOmyART commented 2 years ago

sry, for beeing late to the issue but don't mess with the router logic in the core!
i had a similar problem since i basically wrote my whole app in a package to keep fuel clean and this change in the core broke everything! i also applied the above fix and it introduced a whole lot of other problems!
it was so bad that in the end i had to rewrite everything!

so, if you need any router logic and still separate the code from the main app, put it into a module!

reference: https://fuelphp.com/forums/discussion/15217

WanWizard commented 2 years ago

Then you did it wrong.

Packages are for backend extensions, modules are for frontend extensions. From which follows that packages should never contain routes.