fix getHref strategy in PageMvc #4282

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@vnagara
Contributor
vnagara commented Apr 20, 2013

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
Member

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
Contributor
vnagara commented Apr 22, 2013

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
Member

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
Contributor
vnagara commented Apr 22, 2013

@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
Contributor
vnagara commented Apr 22, 2013

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
Member

@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.

@vnagara
Contributor
vnagara commented Apr 23, 2013
@weierophinney weierophinney commented on the diff Apr 23, 2013
library/Zend/Navigation/Page/Mvc.php
+ */
+ 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
weierophinney Apr 23, 2013 Member

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

@weierophinney weierophinney commented on the diff Apr 23, 2013
library/Zend/Navigation/Page/Mvc.php
@@ -190,7 +197,7 @@ public function getHref()
);
}
- if ($this->getRouteMatch() !== null) {
+ if ($this->getUseRouteMatch() && $this->getRouteMatch() !== null) {
@weierophinney
weierophinney Apr 23, 2013 Member

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

@weierophinney weierophinney commented on the diff Apr 23, 2013
library/Zend/Navigation/Page/Mvc.php
@@ -428,6 +435,30 @@ public function setRouteMatch(RouteMatch $matches)
}
/**
+ * Get the useRouteMatch
+ *
+ * @return bool
+ */
+ public function getUseRouteMatch()
@weierophinney
weierophinney Apr 23, 2013 Member

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

@weierophinney
Member

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

@weierophinney weierophinney added a commit that referenced this pull request Apr 23, 2013
@weierophinney weierophinney [#4282] Incorporate feedback
- s/roteMatch/routeMatch/g
- s/getUseRouteMatch/useRouteMatch/g
eb152de
@weierophinney weierophinney added a commit that referenced this pull request Apr 23, 2013
@weierophinney weierophinney Merge branch 'hotfix/4282' into develop
Forward port #4282
e636ee2
@weierophinney weierophinney added a commit that closed this pull request Apr 23, 2013
@weierophinney weierophinney Merge branch 'hotfix/4282'
Close #4282
c4bf146
@vnagara
Contributor
vnagara commented Apr 23, 2013
@Slamdunk
Contributor

@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
Member

@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
Contributor

Exactly what I mean and what It's needed 😄

@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#4282 from vnagara/fix-…
…getHref-strategy-in-PageMvc

fix getHref strategy in PageMvc
052e328
@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#4282] Incorporate feedback
- s/roteMatch/routeMatch/g
- s/getUseRouteMatch/useRouteMatch/g
f03f89a
@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/4282' 50b28a7
@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
@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