getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Routes defined in plugins are partially overridden by those defined in config.php #490

Closed rasteiner closed 6 years ago

rasteiner commented 6 years ago

Routes in plugins don't work because extend() in trait AppPlugins is called twice - the second time probably not really on purpose since the call stack looks like this:

#0 /kirby/src/Cms/AppPlugins.php(32): Kirby\Cms\Extend::routes(Array, NULL)
#1 /kirby/src/Cms/AppOptions.php(41): Kirby\Cms\App->extend(Array)
#2 /kirby/src/Cms/AppOptions.php(21): Kirby\Cms\App->options()
#3 /kirby/src/Cms/AppErrors.php(41): Kirby\Cms\App->option('debug')
#4 /kirby/src/Cms/AppErrors.php(34): Kirby\Cms\App->handleHtmlErrors()
#5 /kirby/src/Cms/App.php(68): Kirby\Cms\App->handleErrors()
#6 /index.php(5): Kirby\Cms\App->__construct()
#7 {main}

(the class App constructor wants to "handle errors", that one wants to check if the debug config has been set, but by checking that, the options function loads the config files AND also calls extend)

Calling extend() twice breaks stuff because it uses array_replace_recursive() which doesn't really work for the array structure used for routes extensions:

in pseudocode:

array_replace_recursive( 
   { routes: [a, b] }, 
   { routes: [c, d, e] }
)
// returns { routes: [c, d, e]}

This means some routes in your plugin might work, as long as you declare more routes there than in your config file. E.g. if you have 3 routes in your config file, the fourth route in your plugin will work


Kirby\Cms\App->options() probably shouldn't have side effects (like calling extend) and adding the routes from config.php should probably be done explicitly in App.

to make array_replace_recursive() work with routes, each route should have a unique key in the array (could be based on its pattern):

array_replace_recursive( 
   { routes: ['a': a, 'b': b] }, 
   { routes: ['c': c, 'd': d, 'e': e] }
)
// returns { routes: ['a': a, 'b': b, 'c': c, 'd': d, 'e': e]}
bastianallgeier commented 6 years ago

✅ should be fixed on the develop branch