Issues

ZF-10725: ViewRenderer not normalizing action name before constructing view script name

Description

Zend Framework uses a different action name than the view script when using some non-alphanumeric characters in a URL

For example, requesting: {{/somemodule/somecontroller/someaction-}} will (arguably) incorrectly use the {{someactionAction}} method and will then try and use the view script {{someaction-.phtml}}, not find it, and throw a Zend_View_Exception

Likewise with something like: {{/somemodule/somecontroller/someaction.$$$}} Will also fail with a Zend_View_Exception with the message {{script 'someaction-.phtml' not found in path}}

This is due to the different ways that the action is resolved (remove all non-alpha characters) to the way the view script is resolved (replace all non-alpha characters with a '-')

I believe the problem lies within {{Zend_Controller_Action_Helper_ViewRenderer->getViewScript()}} when {{$action = $request->getActionName();}} is called it should be getting the action name from the dispatcher rather than the request.

Comments

I agree that ViewRenderer should retrieve the action name from the dispatcher rather than the request. However, this would mean view script naming convention would now match that of controller method names, a major bc break (existing sites would need to rename their view scripts). Perhaps this modification could/should be pushed up to 2.x for consideration, if it hasn't already been resolved there?

Agreed -- grabbing the action name from the dispatcher is a bad idea in ZF1, as the action name there is (a) camelCased, and (b) suffixed with the word "Action" -- either one of which would alter how view scripts are resolved and represent a BC break.

What I see here is, rather, a request for doing some normalization of action names to ensure they do not begin or end with non-alphanumerics. That could likely be supported in both ZF1 and ZF2, as clearly their inclusion is leading to buggy behavior.

@Matthew, Good point. We can simply strip off the extraneous characters from within getViewScript:


// Remove non-alphanumeric characters from action name
// see ZF-10725
$vars['action'] = preg_replace(
    array('/[^a-z0-9]+$/', '/^[^a-z0-9]+/'),
    array('', ''),
    $vars['action']
);

My preg skills are a bit weak, so there is probably a tidier way of doing it, but that fixes the issue for me. I've attached a patch with this fix and a test case.

Not sure if this is any consequence, but doing this does have some potential to create SEO problems, as these URIs will all point to the same "page":

/my/controller/action
/my/controller/action-
/my/controller/-action-
...

But I digress, as I think that is more related to the dispatcher than to this particular bug.

Fixed in trunk r24154

Patch applied to 1.11 release branch.

@Adam and @Matthew,

Unfortunately, this change causes requests to with actions containing capital letters to break. For example, requests to {{http://www.site.com/Controller/Action}} results in the following error:

{{script 'controller/ction.phtml' not found in path (/path/to/application/views/scripts/)}}

All leading capital letters in the action name are stripped with the current regexes.

Crap crap crap. Forgot to make the regex case-insensitive. It's fixed and unit test updated.

Merged to trunk in r24216

Matthew, could you merge this change into release for me? It appears the fix in the release branch isn't the same as what was committed to trunk, and I don't feel comfortable enough with the merge process yet to do conflict resolution. Thanks!

Re-opened due to comments

Merged r24216 to 1.11 release branch.

@Adam,

The bug still remains. Take, for example, a request to {{/Controller/Action/}}. The patched code executes two regexes to scrub the action name:


$replacePattern = array('/[^a-z0-9]+$/i', '/^[^a-z0-9]+/i');
$vars['action'] = preg_replace($replacePattern, '', $vars['action']);

// Remove non-alphanumeric characters from action name
// see ZF-10725
$vars['action'] = preg_replace(
    array('/[^a-z0-9]+$/', '/^[^a-z0-9]+/'),
    array('', ''),
    $vars['action']
);

Before the first {{preg_replace()}} executes, the {{$var['action']}} has the value {{'Action'}}. After the first {{preg_replace()}}, {{$var['action']}} still has the value {{'Action'}} (though all non-alphanumeric characters would have been removed had there been any). When the second {{preg_replace()}} executes, {{$var['action']}} is reassigned the value {{'ction'}} (where capital 'A' has been stripped).

It seems as if the second {{preg_replace()}} statement is redundant in its current form.

the second regexp is really bad because it's case sensitive, applications that use capitalized action names totally break down - still there in 1.11.9

Oh dear. The patch was double-introduced. I initially committed a cleaned-up version of the patch in the file attached to this issue in r24154:


Index: library/Zend/Controller/Action/Helper/ViewRenderer.php
===================================================================
--- library/Zend/Controller/Action/Helper/ViewRenderer.php      (revision 24153)
+++ library/Zend/Controller/Action/Helper/ViewRenderer.php      (revision 24154)
@@ -626,6 +626,9 @@
         } elseif (null !== $action) {
             $vars['action'] = $action;
         }
+
+        $replacePattern = array('/[^a-z0-9]+$/', '/^[^a-z0-9]+/');
+        $vars['action'] = preg_replace($replacePattern, '', $vars['action']);

         $inflector = $this->getInflector();
         if ($this->getNoController() || $this->getNeverController()) {

However Matthew committed the patch in the file attached to this issue on top of my commit, so we now have two blocks of code performing the same fix (as Brian shows in his comment above). When I fixed the BC break, I must not have pulled down Matthew's commit, as I only saw the first regex and so only fixed that one.

Commits r24199 & r24200 need to be backed out, as they contain an old version of the patch. I've tested this out from a freshly-checked-out trunk and it resolves the issue (./runtests.sh ZF-10725 now passes)

@Matthew: Could you back out those commits for me?

Attached a patch which will remove the duplicate code applied in r24199/r24200 from ZF 1.11.9.

On *nix, you can run the following patch command from the root of your ZF 1.11.9 install:


patch -p0 -i /path/to/ZF-10725_fix_for_1.11.9.patch

New patch applied, as well as a test for the route "Controller/Action" resolving to view script "controller/action.phtml" case presented in the comments. Patch is in trunk and 1.11 release branch; please test.