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

When only modules are used, links with no presenter mask cannot be created with just one RouteList #216

Closed
spaze opened this issue Apr 16, 2019 · 5 comments

Comments

@spaze
Copy link

@spaze spaze commented Apr 16, 2019

Version: dev-master

Bug Description

When only modules are used in an app, links cannot be created when only one RouteList is used. Exception saying "Invalid link: No route for Bar:Foo:waldo()" is thrown.

Steps To Reproduce

  1. Download nette/sandbox
  2. composer install
  3. Move Homepage presenter into a Home module:
-namespace App\Presenters;
+namespace App\HomeModule\Presenters;
  1. Create another module with another presenter (and put it into FooPresenter.php for example):
namespace App\BarModule\Presenters;
final class FooPresenter extends \App\Presenters\BasePresenter
{
	public function renderWaldo(): void
...
  1. Update RouterFactory to read
		$router->addRoute('foobar/<action>', [
			'presenter' => 'Foo',
			'action' => 'default',
			'module' => 'Bar',
		]);
		$router->addRoute('<presenter>/<action>', [
			'presenter' => 'Homepage',
			'action' => 'default',
			'module' => 'Home',
		]);

Note the 'module' key.

  1. Update Homepage/default.latte to include
<a n:href=":Bar:Foo:waldo">bar module</a>
  1. Enable debug mode, edit Bootstrap.php, e.g.:
-		//$configurator->setDebugMode('23.75.345.200'); // enable for your remote IP
+		$configurator->setDebugMode(true); // enable for your remote IP
  1. Start PHP (see the help in nette/sandbox), and load the page

  2. You should see

User Warning
Invalid link: No route for Bar:Foo:waldo()

  1. Change the router (RouterFactory.php) code:
-		$router->addRoute('foobar/<action>', [
-			'presenter' => 'Foo',
-			'action' => 'default',
-			'module' => 'Bar',
-		]);
-		$router->addRoute('<presenter>/<action>', [
-			'presenter' => 'Homepage',
-			'action' => 'default',
-			'module' => 'Home',
-		]);

+		$routerBar = new RouteList('Bar');
+		$routerBar->addRoute('foobar/<action>', [
+			'presenter' => 'Foo',
+			'action' => 'default',
+		]);
+		$router->add($routerBar);
+
+		$routerHome = new RouteList('Home');
+		$routerHome->addRoute('<presenter>/<action>', [
+			'presenter' => 'Homepage',
+			'action' => 'default',
+		]);
+		$router->add($routerHome);

Note that module name was moved from the module key to the RouteList constructor and that now, instead of adding a Route, we add a RouteList.

  1. Load the page in your browser again, no error

Expected Behavior

It works :-) Or the exception is more clear or docs are clear.

Possible Solution

There are several options, not sure which one is the correct one, feedback wanted:

  • update docs and say when only modules are used then each module needs to use its own RouteList with the module name specified in the constructor
  • somehow try to make it work as it used to before nette/routing was created

Spent quite some time debugging the problem and this is what helped to fix (and understand) the problem for me:

#208 describes that nette/routing classes have no idea about //modules// and that Application\Routers\RouteList solves that. But this is in constructUrl:

	public function constructUrl(array $params, Nette\Http\UrlScript $refUrl): ?string
	{
		if ($this->module) {
			if (strncmp($params[self::PRESENTER_KEY], $this->module, strlen($this->module)) === 0) {
				$params[self::PRESENTER_KEY] = substr($params[self::PRESENTER_KEY], strlen($this->module));
			} else {
				return null;
			}
		}

		return parent::constructUrl($params, $refUrl);
	}

so if there's no $this->module then it just calls parent::constructUrl which doesn't know anything about modules.

The docs says:

In Nette we can split presenters into modules. Therefore we need to work with those modules in routes. We can use module parameter in Route class:
+
If we have more routes that we want to group together in a module, we can use RouteList with name of module in constructor:

So I tried using the constructor param and it worked.

Honestly I'd be totally fine with just updating the docs (and maybe add a BC warning to nette/application release notes?), let me know I can fix it (just not sure how).

Thanks and sorry for this long bug report, tried to write down all I've learned in the past 2 days or so.

spaze added a commit to spaze/michalspacek.cz that referenced this issue Apr 18, 2019
@spaze

This comment has been minimized.

Copy link
Author

@spaze spaze commented Apr 24, 2019

Here's a failing test https://github.com/spaze/application/commit/acfed5df8097de070bc805e65c117cd399b0e523
(also attaching as a file, remove the .txt extension), it fails with $list = new RouteList;, not with $list = new RouteList('Auth');.

It fails on master, doesn't fail on the 2.4 branch. And seems the problem is triggered by routes that don't specify a presenter mask (<presenter>) but instead would always use the default presenter specified in the metadata array.

@spaze spaze changed the title When only modules are used, links cannot be created with just one RouteList When only modules are used, links with no presenter mask cannot be created with just one RouteList Apr 24, 2019
@JanMikes

This comment has been minimized.

Copy link

@JanMikes JanMikes commented Oct 21, 2019

@spaze

This comment has been minimized.

Copy link
Author

@spaze spaze commented Oct 21, 2019

Hi @JanMikes, quite possibly. I've done something similar as what you describe in the forum post, and that was use multiple routelists, one per module (see this commit, the commit message is enough, the diff is specific to my site so it won't help anyone I guess)

@dg dg closed this in 0856f56 Oct 21, 2019
@spaze

This comment has been minimized.

Copy link
Author

@spaze spaze commented Oct 21, 2019

Thanks @dg!

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Oct 21, 2019

Sorry for the delay, somehow I missed it.

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