Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Url View-helper: better exception wording on router->assemble() fail #200

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Url View-helper: better exception wording on router->assemble() fail #200

wants to merge 5 commits into from

Conversation

MatthiasKuehneEllerhold
Copy link
Contributor

This PR will change 2 things:

  1. Add a comment to the HelperPluginManager that the "url" and "basePath" View-Helper factories are overwritten in the ViewHelperManagerFactory of zend-mvc.

  2. Change the exception text of the "url" View-helper if the router cant assemble the URL to include more information:

Before:

Zend\Router\Exception\InvalidArgumentException: Missing parameter "my-parameter" in /var/www/vendor/zendframework/zend-router/src/Http/Segment.php:309
Stack trace:
#0 /var/www/vendor/zendframework/zend-router/src/Http/Segment.php(419): Zend\Router\Http\Segment->buildPath(Array, Array, false, false, Array)
#1 /var/www/vendor/zendframework/zend-router/src/Http/TreeRouteStack.php(370): Zend\Router\Http\Segment->assemble(Array, Array)
#2 /var/www/vendor/zendframework/zend-router/src/Http/Part.php(216): Zend\Router\Http\TreeRouteStack->assemble(Array, Array)
#3 /var/www/vendor/zendframework/zend-router/src/Http/TreeRouteStack.php(370): Zend\Router\Http\Part->assemble(Array, Array)
#4 /var/www/vendor/zendframework/zend-router/src/Http/Part.php(216): Zend\Router\Http\TreeRouteStack->assemble(Array, Array)
#5 /var/www/vendor/zendframework/zend-router/src/Http/TreeRouteStack.php(370): Zend\Router\Http\Part->assemble(Array, Array)
#6 /var/www/vendor/zendframework/zend-router/src/Http/Part.php(216): Zend\Router\Http\TreeRouteStack->assemble(Array, Array)
#7 /var/www/vendor/zendframework/zend-router/src/Http/TreeRouteStack.php(391): Zend\Router\Http\Part->assemble(Array, Array)
#8 /var/www/vendor/zendframework/zend-mvc-i18n/src/Router/TranslatorAwareTreeRouteStack.php(111): Zend\Router\Http\TreeRouteStack->assemble(Array, Array)
#9 /var/www/vendor/zendframework/zend-view/src/Helper/Url.php(106): Zend\Mvc\I18n\Router\TranslatorAwareTreeRouteStack->assemble(Array, Array)
#10 [internal function]: Zend\View\Helper\Url->__invoke('my/route/...', Array, Array, false)
#11 /var/www/vendor/zendframework/zend-view/src/Renderer/PhpRenderer.php(397): call_user_func_array(Object(Zend\View\Helper\Url), Array)
#12 /var/www/module/xxx/view/xxx/yyy.phtml(101): Zend\View\Renderer\PhpRenderer->__call('url', Array)
...

After:

Zend\View\Helper\Exception\InvalidArgumentException: Couldnt create URL for route "my/route/do-it", params "{}" and options "[]" in /var/www/vendor/zendframework/zend-view/src/Helper/Url.php:109
Stack trace:
#0 [internal function]: Zend\View\Helper\Url->__invoke('my/route/...', Array)
#1 /var/www/vendor/zendframework/zend-view/src/Renderer/PhpRenderer.php(397): call_user_func_array(Object(Zend\View\Helper\Url), Array)
#2 /var/www/module/xxx/view/xxx/yyy.phtml(101): Zend\View\Renderer\PhpRenderer->__call('url', Array)
...

Advantages:

  • full route name clearly visible
  • full parameters list clearly visible
  • the calling view-script is way higher in the stack trace and therefore easier to find (see place 2 vs 12)

Open questions:

  • Use json_encode? We'd have to add ext-json to the composer.json ...

@michalbundyra
Copy link
Member

@MatthiasKuehneEllerhold Thanks for your contribution.
We need unit tests to cover these changes. Thanks !

src/Helper/Url.php Outdated Show resolved Hide resolved
@MatthiasKuehneEllerhold
Copy link
Contributor Author

The zend-router only throws the Zend\Mvc\Router exceptions if its 2.x. Thats only tested in the DEPS=lowest setting on travis, see here: https://travis-ci.org/zendframework/zend-view/jobs/611765820?utm_medium=notification&utm_source=github_status

Cant seem to replicate it locally though. But i dont have 7.1 set up anymore.

So the Zend\Mvc\Router exceptions will be tested at DEPS=lowest while the Zend\Router will be tested at DEPS=latest.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-view; a new issue has been opened at laminas/laminas-view#2.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-view. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-view to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-view.
  • In your clone of laminas/laminas-view, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants