Expand routeBuilder unit test

#1331486-239: Move module_invoke_*() and friends to an Extensions class is committed.

The "replace module_list()" bit was done as part of #1331486: Move module_invoke_*() and friends to an Extensions class, so we just need to write the tests.

.

While writing the tests I actually realized that the it is actually not true that the routes are not in memory all the time.
The test file was just some random test writing until I realized that they actually should not pass. Note: At the end all routes
are in the dumper.

Note: RouteCollection->addCollection does not remove old entries.

RouteBuilder says things like this: $yaml_discovery = new YamlDiscovery('routing', $this->moduleHandler->getModuleDirectories());

I say things like this: #2095037: Add vfsStream as a dependency in composer.json.

There we go, this fixes the actual bug as well as tests the rest of the class.

Some fixes later.

There we go.

Let's be honest.

Fixed a missing whitespace.

Discussed with dawehner in IRC. We don't need to mess with the interface for MatcherDumper; there was a bug in the tests he was using. :-)

We have been talking about removing the matcher dumper to be a service, thought this is not part of the patch as it would be kind of out of scope.

Fixed the errors.

Overall, that is some good unit testing.

1. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -113,4 +121,17 @@ public function rebuild() {
   +   * Returns the yaml discovery for getting all the .routing.yml files.
   

   Should we use YAML?

2. +++ b/core/tests/Drupal/Tests/Core/Routing/RouteBuilderTest.php
   @@ -0,0 +1,240 @@
   +    $this->assertNull($this->routeBuilder->rebuild());
   

   Won't this always return NULL anyway? regardless of whether the lock fails.

3. +++ b/core/tests/Drupal/Tests/Core/Routing/RouteBuilderTest.php
   @@ -0,0 +1,240 @@
   +      ->with($this->equalTo(RoutingEvents::ALTER), $this->isInstanceOf('Drupal\Core\Routing\RouteBuildEvent'));
   

   Nice!

Thanks for the review!

Won't this always return NULL anyway? regardless of whether the lock fails.

Someone had the idea that phpunit strict mode would be a good idea.

:) Maybe we should make rebuild() return something?

Well, we could just return TRUE/FALSE but it is kind of pointless to be honest.

Here is a patch for that.

+++ b/core/lib/Drupal/Core/Routing/MatcherDumperInterface.php
@@ -0,0 +1,26 @@
+/**
+ * Extends the symfony matcher dumper interface with a setRoutes method.
+ */
+interface MatcherDumperInterface extends SymfonyMatcherDumperInterface {
+
+  /**
+   * Adds additional routes to be dumped.
+   *
+   * @param \Symfony\Component\Routing\RouteCollection $routes
+   *   A collection of routes to add to this dumper.
+   */
+  public function addRoutes(RouteCollection $routes);
+
+}

Why are we still doing this? It's not needed.

Why are we still doing this? It's not needed.

I potentially agree that it is scope-creep though addRoutes is not part of the symfony interface, so as the router
system should be possible to be used on whatever other database system we should introduce an interface. Do you want to add a new issue for that or is it okay in this issue?

#23: route_rebuild-1859684-23.patch queued for re-testing.

1. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -56,12 +62,14 @@ class RouteBuilder {
   +   * @param \Drupal\Core\Routing\MatcherDumperInterface|\Symfony\Component\Routing\Matcher\Dumper\MatcherDumperInterface $dumper
   

   why the "|" OR here? the first is a child of the second, so just use the second.

2. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -72,6 +80,9 @@ public function __construct(MatcherDumperInterface $dumper, LockBackendInterface
   +   *   Returns TRUE if the rebuild succeeds, otherwise FALSE.
   

   should be "FALSE otherwise".

Can't say much about the rest, looks like we are merely expanding test coverage here? No actual bug is fixed here?

why the "|" OR here? the first is a child of the second, so just use the second.

Ah this was not on purpose.

Can't say much about the rest, looks like we are merely expanding test coverage here? No actual bug is fixed here?

The only actual bug was the missing interface.

We should drop addRoutes(). The dumper not being a true-service is a separate matter (and my fault for not catching originally). Adding another method that duplicates existing functionality is not necessary.

So you suggest to just leave the method but just skip to use an interface?

Never mind, I was misreading the patch. :-) Ignore my objections.

+/**
+ * Extends the symfony matcher dumper interface with a setRoutes method.
+ */
+interface MatcherDumperInterface extends SymfonyMatcherDumperInterface {
+
+  /**
+   * Adds additional routes to be dumped.
+   *
+   * @param \Symfony\Component\Routing\RouteCollection $routes
+   *   A collection of routes to add to this dumper.
+   */
+  public function addRoutes(RouteCollection $routes);
+
+}

Where is this setRoutes() method that's supposed to be added?

@catch
I was wrong on that. All we need is the already existing addRoutes method.

In that case, we should update the comment so it's not talking about a method that doesn't exist, and I'm wondering why we're extending the interface at all if it has exactly the same single identical method to the Symfony one. If there's a reason it could use a comment as to why.

Added some comment about the need for an interface: https://drupal.org/node/1859684

Background: The Symfony dumper interface doesn't define how routes get queued up to be dumped, just how they're dumped. Our dumper defines a method for doing so, but when I wrote it I didn't bother to make an interface for it. (The Symfony default one uses a constructor parameter for a single RouteCollection, which wouldn't scale for us.) This issue fixes that oversight on my part.

I was confused by this too, earlier in the thread. We're good now.

Sorry I still don't get it.

The comment says

Extends the symfony matcher dumper interface with a setRoutes method.

However the interface does not extend the interface with a setRoutes method, it just defines an addRoutes() method.

If I'm missing something please let me know, but I don't see how the patch is possibly correct at the moment.

I think that's just the comment being a bit out of date. It should now read addRoutes(). Does that make more sense?

Yes that's all I really wanted in #32 (assuming that was the problem).

Oh sometimes I am just blind.

Seems to happen to a lot of people in this thread. :-)

Committed/pushed to 8.x, thanks!

Let's also mark the issue as fixed.