Skip to content

[FrameworkBundle] Improving bad bundle exception in _controller #11210

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 8, 2014
Merged

[FrameworkBundle] Improving bad bundle exception in _controller #11210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 48 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/Controller/ControllerNameParser.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public function __construct(KernelInterface $kernel)
*/
public function parse($controller)
{
$originalController = $controller;
if (3 != count($parts = explode(':', $controller))) {
throw new \InvalidArgumentException(sprintf('The "%s" controller is not a valid "a:b:c" controller string.', $controller));
}
Expand All @@ -54,8 +55,24 @@ public function parse($controller)
$controller = str_replace('/', '\\', $controller);
$bundles = array();

// this throws an exception if there is no such bundle
foreach ($this->kernel->getBundle($bundle, false) as $b) {
try {
// this throws an exception if there is no such bundle
$allBundles = $this->kernel->getBundle($bundle, false);
} catch (\InvalidArgumentException $e) {
$message = sprintf(
'The "%s" (from the _controller value "%s") does not exist or is not enabled in your kernel!',
$bundle,
$originalController
);

if ($alternative = $this->findAlternative($bundle)) {
$message .= sprintf(' Did you mean "%s:%s:%s"?', $alternative, $controller, $action);
}

throw new \InvalidArgumentException($message, 0, $e);
}

foreach ($allBundles as $b) {
$try = $b->getNamespace().'\\Controller\\'.$controller.'Controller';
if (class_exists($try)) {
return $try.'::'.$action.'Action';
Expand Down Expand Up @@ -100,4 +117,33 @@ public function build($controller)

throw new \InvalidArgumentException(sprintf('Unable to find a bundle that defines controller "%s".', $controller));
}

/**
* Attempts to find a bundle that is *similar* to the given bundle name
*
* @param string $nonExistentBundleName
* @return string
*/
private function findAlternative($nonExistentBundleName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing phpdoc

{
$bundleNames = array_map(function ($b) {
return $b->getName();
}, $this->kernel->getBundles());

$alternative = null;
$shortest = null;
foreach ($bundleNames as $bundleName) {
// if there's a partial match, return it immediately
if (false !== strpos($bundleName, $nonExistentBundleName)) {
return $bundleName;
}

$lev = levenshtein($nonExistentBundleName, $bundleName);
if ($lev <= strlen($nonExistentBundleName) / 3 && ($alternative === null || $lev < $shortest)) {
$alternative = $bundleName;
}
}

return $alternative;
}
}
34 changes: 34 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerNameParserTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,36 @@ public function getMissingControllersTest()
);
}

/**
* @expectedException
* @dataProvider getInvalidBundleNameTests
*/
public function testInvalidBundleName($bundleName, $suggestedBundleName)
{
$parser = $this->createParser();

try {
$parser->parse($bundleName);
} catch (\Exception $e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that you can all exceptions, this will never fail if there is no exception (PHPUnit failures are exception).

You should use the PHPUnit expectedException feature instead, which takes care of this

$this->assertInstanceOf('\InvalidArgumentException', $e, '->parse() throws a \InvalidArgumentException if the bundle does not exist');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not actually test the new feature. You need to test the message


if (false === $suggestedBundleName) {
// make sure we don't have a suggestion
$this->assertNotContains('Did you mean', $e->getMessage());
} else {
$this->assertContains(sprintf('Did you mean "%s"', $suggestedBundleName), $e->getMessage());
}
}
}

public function getInvalidBundleNameTests()
{
return array(
array('FoodBundle:Default:index', 'FooBundle:Default:index'),
array('CrazyBundle:Default:index', false),
);
}

private function createParser()
{
$bundles = array(
Expand All @@ -121,6 +151,10 @@ private function createParser()
->expects($this->any())
->method('getBundle')
->will($this->returnCallback(function ($bundle) use ($bundles) {
if (!isset($bundles[$bundle])) {
throw new \InvalidArgumentException(sprintf('Invalid bundle name "%s"', $bundle));
}

return $bundles[$bundle];
}))
;
Expand Down