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

RoutingExtension: added support for custom single-route classes. #162

Merged
merged 1 commit into from May 17, 2017

Conversation

Projects
None yet
3 participants
@vlastavesely
Copy link
Contributor

commented Oct 3, 2016

An alternative way to add new Route to RouteList. It moves control over the Route class into the RouteList, so Neon configuration for RoutingExtension can be used by other combination of classes descended from Nette*\RouteList and Nette*\Route.

@dg

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

What is it good for?

@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2016

With this, complex "drop-in replacement" routers can be made using existing declaration for RoutingExtension.

Now, when the 'routing.router' is replaced by other class that depends e.g. on a translator, its children remain to be Nette*\Route that does not know what to do with that translator. You cannot set filters to $metadata in config.neon, so you have to replace routes with class that does it inside itself.

If my PR accepted, there is possible to simply override addRoute() where child can get its dependencies - not even for translation but e. g. for DB filtering, etc. This solution keeps freedom to declare routes both in bootstrap and config.neon. Otherwise, configuration for RoutingExtension could not be used in this case and everything would have to be done in the bootstrap.php.

@rostenkowski

This comment has been minimized.

Copy link

commented Oct 6, 2016

👍

@dg

This comment has been minimized.

Copy link
Member

commented Oct 6, 2016

So it is not about RouteList, it is about RoutingExtension. I see RoutingExtension as experimental and I welcome any improvements. These improvements, that solves your use case, should be IMHO done in RoutingExtension.

@vlastavesely vlastavesely force-pushed the vlastavesely:master branch from 7e61b79 to 5157fa1 Oct 7, 2016

@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2016

I do agree. After your suggestion I've tried to find some better solution and I think I've found one.

In many cases (even in Nette and my project) RouteList and Route will be located in the same namespace. So, it would be sufficient to create routes from class Route located in the same namespace the RouteList belongs to. It not only solves my problem but also it allows me to leave my config.neon without any changes. To make it even more useful, I have added possibility to change child route's class totally by parameter routing.routeClass.

This solution has, though, one limitation. It cannot pass dependencies via constructor to child routes. These have to be set later by calling e.g. Route::setTranslator(). But this is not problem for functionality.

@vlastavesely vlastavesely changed the title RouteList: Added support for using config of RouterExtension by descendants of RouteList RoutingExtension: added support for custom single-route classes. Oct 7, 2016

@vlastavesely vlastavesely force-pushed the vlastavesely:master branch from 5157fa1 to 6b79913 Oct 19, 2016

@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2016

Updated. Class Route from RouteList's namespace is used only if exists.

@dg dg force-pushed the nette:master branch 4 times, most recently from 7447ad4 to 3165d3a Dec 20, 2016

@dg dg force-pushed the nette:master branch 4 times, most recently from 3fb5d4a to d2e3dc1 Jan 2, 2017

@dg dg force-pushed the nette:master branch 11 times, most recently from 07d4ebb to 337c67c Jan 14, 2017

@dg dg force-pushed the nette:master branch 2 times, most recently from 3a6260c to 1d25e25 Jan 25, 2017

@dg dg force-pushed the nette:master branch 16 times, most recently from d186075 to 4fedb44 Jan 25, 2017

@dg dg force-pushed the nette:master branch from 88ef0bd to b7df270 Feb 2, 2017

@dg dg force-pushed the nette:master branch from e6b9262 to af754ba Feb 9, 2017

if ($this->config['routeClass']) {
$class = $this->config['routeClass'];
} else {
$namespace = Nette\PhpGenerator\Helpers::extractNamespace($router->getFactory()->getEntity());

This comment has been minimized.

Copy link
@dg

dg May 2, 2017

Member

This is BC break, isn't it?

This comment has been minimized.

Copy link
@vlastavesely

vlastavesely May 2, 2017

Author Contributor

In the case that a class Route exists in the same NS as RouteListm, yes. But this magic could be removed and used $this->config['routeClass'] if it is set or Nette\Application\Routers\Route::class otherwise. In that case, there should not be any BC.

This comment has been minimized.

Copy link
@dg

dg May 17, 2017

Member

So can you change it?

@vlastavesely vlastavesely force-pushed the vlastavesely:master branch from 6b79913 to 1ee81ed May 17, 2017

@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

done

@@ -55,6 +52,13 @@ public function loadConfiguration()
public function beforeCompile()
{
$builder = $this->getContainerBuilder();
$router = $builder->getDefinition($this->prefix('router'));

This comment has been minimized.

Copy link
@dg

dg May 17, 2017

Member

$router is not needed, or is it? It can be moved back to loadConfiguration() IMHO.

This comment has been minimized.

Copy link
@vlastavesely

vlastavesely May 17, 2017

Author Contributor

:( yes, you're right. it had reason in previous version but after the last changes, it can be moved back into loadConfiguration. I am gonna change it withing a few minutes.

This comment has been minimized.

Copy link
@dg

dg May 17, 2017

Member

Thanks

@vlastavesely vlastavesely force-pushed the vlastavesely:master branch from 1ee81ed to 05bd323 May 17, 2017

@vlastavesely

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

done

@dg

This comment has been minimized.

Copy link
Member

commented May 17, 2017

Thanks

@dg dg merged commit b5f19f3 into nette:master May 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 80.336%
Details

dg added a commit that referenced this pull request May 17, 2017

dg added a commit that referenced this pull request May 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.