Skip to content

Commit

Permalink
bug #16477 [Routing] Changing RouteCollectionBuilder::import() behavi…
Browse files Browse the repository at this point in the history
…or to add to the builder (weaverryan)

This PR was squashed before being merged into the 2.8 branch (closes #16477).

Discussion
----------

[Routing] Changing RouteCollectionBuilder::import() behavior to add to the builder

| Q             | A
| ------------- | ---
| Bug fix?      | behavior change
| New feature?  | behavior change
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Based on conversation starting here: #15990 (comment).

```php
// Before:
$routes->mount('/admin', $routes->import(__DIR__.'/config/admin.yml');

// After:
$routes->import(__DIR__.'/config/admin.yml', '/admin');
```

This makes `import()` actually add the `RouteCollectionBuilder` into itself. We didn't do this before at Fabien's request, and actually the current implementation (before this PR) is quite "clean". However, I agree with @wouterj that `import()` really sounds/looks like it will actually *import* those routes *into* this `RouteCollectionBuilder`.

This change is subjective - we just need to pick which way we like better and run full steam with it.

Commits
-------

8feb9ef [Routing] Changing RouteCollectionBuilder::import() behavior to add to the builder
  • Loading branch information
fabpot committed Nov 23, 2015
2 parents fe6e9e4 + 8feb9ef commit 57e0468
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 6 deletions.
12 changes: 8 additions & 4 deletions src/Symfony/Component/Routing/RouteCollectionBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,17 @@ public function __construct(LoaderInterface $loader = null)
/**
* Import an external routing resource and returns the RouteCollectionBuilder.
*
* $routes->mount('/blog', $routes->import('blog.yml'));
* $routes->import('blog.yml', '/blog');
*
* @param mixed $resource
* @param string $type
* @param mixed $resource
* @param string|null $prefix
* @param string $type
*
* @return RouteCollectionBuilder
*
* @throws FileLoaderLoadException
*/
public function import($resource, $type = null)
public function import($resource, $prefix = '/', $type = null)
{
/** @var RouteCollection $collection */
$collection = $this->load($resource, $type);
Expand All @@ -73,6 +74,9 @@ public function import($resource, $type = null)
$builder->addResource($resource);
}

// mount into this builder
$this->mount($prefix, $builder);

return $builder;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public function testImport()

// import the file!
$routes = new RouteCollectionBuilder($loader);
$importedRoutes = $routes->import('admin_routing.yml', 'yaml');
$importedRoutes = $routes->import('admin_routing.yml', '/', 'yaml');

// we should get back a RouteCollectionBuilder
$this->assertInstanceOf('Symfony\Component\Routing\RouteCollectionBuilder', $importedRoutes);
Expand All @@ -56,6 +56,9 @@ public function testImport()
$this->assertSame($originalRoute, $route);
// should return file_resource.yml, which is in the original collection
$this->assertCount(1, $addedCollection->getResources());

// make sure the routes were imported into the top-level builder
$this->assertCount(1, $routes->build());
}

/**
Expand Down Expand Up @@ -285,7 +288,7 @@ public function testFlushSetsPrefixedWithMultipleLevels()
->method('load')
->will($this->returnValue($importedCollection));
// import this from the /admin route builder
$adminRoutes->mount('/imported', $adminRoutes->import('admin.yml'));
$adminRoutes->import('admin.yml', '/imported');

$collection = $routes->build();
$this->assertEquals('/admin/dashboard', $collection->get('admin_dashboard')->getPath(), 'Routes before mounting have the prefix');
Expand Down

0 comments on commit 57e0468

Please sign in to comment.