Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

fix getHref strategy in PageMvc #4282

Closed
wants to merge 5 commits into from

3 participants

@vnagara

I was caused by problem which breaks menu link dependent from which page it was generated.

As example in one router I have default action 'list' in another - 'view'. Current getHref function replace proper default action parameter with broken from route match.
Also if I will add some new defaults then I should has to fix all entrances in navigation array.

@weierophinney

A couple comments:

  • For configuration values, we usually prefer "underscore_separated" names, as we can normalize them more easily. As such, I'd recommend naming the config value "use_route_match".
  • You've altered existing test cases, which is generally a sign of a behavior change, and thus potentially a backwards compatibility break. I think we need to have distinct test cases for the behavior both with and without the flag. Ideally, the original test cases should be left intact.
@vnagara

OK, I renamed it to underscore.
About second: It was introduced in v2.1.5 which caused troubles in navigation links and broke BC. I merely want to return this compatibility. So should I change others test functions?
@weierophinney thank for comment.

@weierophinney

About second: It was introduced in v2.1.5 which caused troubles in navigation links and broke BC. I merely want to return this compatibility.

You hadn't mentioned that in the description. :)

If that's the case, then I'd argue the boolean true value of the flag should be the default behavior? If so, that's how it should be coded.

@vnagara

@weierophinney Yeah, really, there should be applied more standards(I did fast fix for compatibility). Do you agree that default value of this param should be false?
At first I want do discuss what should be done than I could do it and in documentation too.
I've just mentioned why I'd used camel-case param, because such is used for 'routeMatch" param (http://zf2.readthedocs.org/en/latest/modules/zend.navigation.pages.html). Also doctrine use camel case params.

@vnagara

ViewHelper has camel-case options - http://zf2.readthedocs.org/en/latest/modules/zend.navigation.view.helper.menu.html. So I propose to set 'useRouteMath' param to camel-cased too.

@weierophinney

@vnagara Ugh -- thought we'd refactored all those before 2.0. Since we didn't, keep the casing consistent within the component.

As for the default value of the parameter -- the default should be whatever value will keep the original behavior prior to the changes in 2.1.5. The configuration value can then be passed when somebody wants to override the behavior.

@weierophinney weierophinney commented on the diff
library/Zend/Navigation/Page/Mvc.php
((7 lines not shown))
+ */
+ public function getUseRouteMatch()
+ {
+ return $this->useRouteMatch;
+ }
+
+ /**
+ * Set whether the page should use route match params for assembling link uri
+ *
+ * @see getHref()
+ * @param bool $useRoteMatch [optional]
+ * @return Mvc
+ */
+ public function setUseRouteMatch($useRoteMatch = true)
+ {
+ $this->useRouteMatch = (bool) $useRoteMatch;
@weierophinney Owner

s/Rote/Route/g (I can do this on merge.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Navigation/Page/Mvc.php
@@ -190,7 +197,7 @@ public function getHref()
);
}
- if ($this->getRouteMatch() !== null) {
+ if ($this->getUseRouteMatch() && $this->getRouteMatch() !== null) {
@weierophinney Owner

It looks like a null value would be highly unlikely here; I think you can omit the extra condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney weierophinney commented on the diff
library/Zend/Navigation/Page/Mvc.php
@@ -428,6 +435,30 @@ public function setRouteMatch(RouteMatch $matches)
}
/**
+ * Get the useRouteMatch
+ *
+ * @return bool
+ */
+ public function getUseRouteMatch()
@weierophinney Owner

For boolean getters like this, we can drop the get prefix, and it becomes simply useRouteMatch().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@weierophinney

Looks good -- I'll make the changes I've noted in suggestions already as I merge.

@weierophinney weierophinney referenced this pull request from a commit
@weierophinney weierophinney [#4282] Incorporate feedback
- s/roteMatch/routeMatch/g
- s/getUseRouteMatch/useRouteMatch/g
eb152de
@Slamdunk

@weierophinney yes, this PR resolves #4095 bug.

But also restores the old behaviour, deleting #4095 intentions, in fact testMvcPageParamsInheritRouteMatchParams and testInheritedRouteMatchParamsWorkWithModuleRouteListener had to be changed.

I suggest to @adamlundrigan to play better with breadcrumbs view helper to reach his goal; now there's no more need to change Zend\Navigation\* anymore; a custom Zend\View\Helper\Navigation\* will be enough.

@weierophinney

@Slamdunk The goal was to restore the pre-2.1.5 behavior, so it sounds like that has happened. It also allows the same behavior that @adamlundrigan was trying to achieve via a flag.

@Slamdunk

Exactly what I mean and what It's needed :smile:

@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney [#4282] Incorporate feedback
- s/roteMatch/routeMatch/g
- s/getUseRouteMatch/useRouteMatch/g
86edbdd
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'hotfix/4282'
Close #4282
d31e6df
@ghost Unknown referenced this pull request from a commit
@weierophinney weierophinney Merge branch 'hotfix/4282' into develop
Forward port #4282
cad462f
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-navigation
@weierophinney weierophinney Merge pull request zendframework/zf2#4282 from vnagara/fix-getHref-st…
…rategy-in-PageMvc

fix getHref strategy in PageMvc
052e328
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-navigation
@weierophinney weierophinney [zendframework/zf2#4282] Incorporate feedback
- s/roteMatch/routeMatch/g
- s/getUseRouteMatch/useRouteMatch/g
f03f89a
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-navigation
@weierophinney weierophinney Merge branch 'hotfix/4282' 50b28a7
@weierophinney weierophinney referenced this pull request from a commit in zendframework/zend-navigation
@weierophinney weierophinney Merge branch 'hotfix/4282' into develop 0eabc04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
33 library/Zend/Navigation/Page/Mvc.php
@@ -76,6 +76,13 @@ class Mvc extends AbstractPage
protected $routeMatch;
/**
+ * If true and set routeMatch than getHref will use routeMatch params
+ * to assemble uri
+ * @var bool
+ */
+ protected $useRouteMatch = false;
+
+ /**
* Router for assembling URLs
*
* @see getHref()
@@ -190,7 +197,7 @@ public function getHref()
);
}
- if ($this->getRouteMatch() !== null) {
+ if ($this->getUseRouteMatch() && $this->getRouteMatch() !== null) {
@weierophinney Owner

It looks like a null value would be highly unlikely here; I think you can omit the extra condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
$rmParams = $this->getRouteMatch()->getParams();
if (isset($rmParams[ModuleRouteListener::ORIGINAL_CONTROLLER])) {
@@ -428,6 +435,30 @@ public function setRouteMatch(RouteMatch $matches)
}
/**
+ * Get the useRouteMatch
+ *
+ * @return bool
+ */
+ public function getUseRouteMatch()
@weierophinney Owner

For boolean getters like this, we can drop the get prefix, and it becomes simply useRouteMatch().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ return $this->useRouteMatch;
+ }
+
+ /**
+ * Set whether the page should use route match params for assembling link uri
+ *
+ * @see getHref()
+ * @param bool $useRoteMatch [optional]
+ * @return Mvc
+ */
+ public function setUseRouteMatch($useRoteMatch = true)
+ {
+ $this->useRouteMatch = (bool) $useRoteMatch;
@weierophinney Owner

s/Rote/Route/g (I can do this on merge.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $this->hrefCache = null;
+ return $this;
+ }
+
+ /**
* Get the router.
*
* @return null|RouteStackInterface
View
34 tests/ZendTest/Navigation/Page/MvcTest.php
@@ -538,14 +538,34 @@ public function testNoExceptionForGetHrefIfDefaultRouterIsSet()
$page->setDefaultRouter(null);
}
+ public function testBoolSetAndGetUseRouteMatch()
+ {
+ $page = new Page\Mvc(array(
+ 'useRouteMatch' => 2,
+ ));
+ $this->assertSame(true, $page->getUseRouteMatch());
+
+ $page->setUseRouteMatch(null);
+ $this->assertSame(false, $page->getUseRouteMatch());
+
+ $page->setUseRouteMatch(false);
+ $this->assertSame(false, $page->getUseRouteMatch());
+
+ $page->setUseRouteMatch(true);
+ $this->assertSame(true, $page->getUseRouteMatch());
+
+ $page->setUseRouteMatch();
+ $this->assertSame(true, $page->getUseRouteMatch());
+ }
+
public function testMvcPageParamsInheritRouteMatchParams()
{
$page = new Page\Mvc(array(
'label' => 'lollerblades',
- 'route' => 'lollerblades'
+ 'route' => 'lollerblades',
));
- $route = new SegmentRoute('/lollerblades/view/:serialNumber');
+ $route = new SegmentRoute('/lollerblades/view[/:serialNumber]');
$router = new TreeRouteStack;
$router->addRoute('lollerblades', $route);
@@ -558,6 +578,9 @@ public function testMvcPageParamsInheritRouteMatchParams()
$page->setRouter($router);
$page->setRouteMatch($routeMatch);
+ $this->assertEquals('/lollerblades/view', $page->getHref());
+
+ $page->setUseRouteMatch(true);
$this->assertEquals('/lollerblades/view/23', $page->getHref());
}
@@ -565,10 +588,10 @@ public function testInheritedRouteMatchParamsWorkWithModuleRouteListener()
{
$page = new Page\Mvc(array(
'label' => 'mpinkstonwashere',
- 'route' => 'lmaoplane'
+ 'route' => 'lmaoplane',
));
- $route = new SegmentRoute('/lmaoplane/:controller');
+ $route = new SegmentRoute('/lmaoplane[/:controller]');
$router = new TreeRouteStack;
$router->addRoute('lmaoplane', $route);
@@ -589,6 +612,9 @@ public function testInheritedRouteMatchParamsWorkWithModuleRouteListener()
$page->setRouter($event->getRouter());
$page->setRouteMatch($event->getRouteMatch());
+ $this->assertEquals('/lmaoplane', $page->getHref());
+
+ $page->setUseRouteMatch(true);
$this->assertEquals('/lmaoplane/index', $page->getHref());
}
}
Something went wrong with that request. Please try again.