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

fix getHref strategy in PageMvc #4282

Closed

Conversation

vnagara
Copy link
Contributor

@vnagara 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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

vnagara commented Apr 23, 2013

@weierophinney done

*/
public function setUseRouteMatch($useRoteMatch = true)
{
$this->useRouteMatch = (bool) $useRoteMatch;
Copy link
Member

Choose a reason for hiding this comment

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

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

@weierophinney
Copy link
Member

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

@ghost ghost assigned weierophinney Apr 23, 2013
weierophinney added a commit that referenced this pull request Apr 23, 2013
weierophinney added a commit that referenced this pull request Apr 23, 2013
- s/roteMatch/routeMatch/g
- s/getUseRouteMatch/useRouteMatch/g
weierophinney added a commit that referenced this pull request Apr 23, 2013
@vnagara
Copy link
Contributor Author

vnagara commented Apr 23, 2013

@weierophinney Thank

@Slamdunk
Copy link
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
Copy link
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
Copy link
Contributor

Exactly what I mean and what It's needed 😄

weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
…getHref-strategy-in-PageMvc

fix getHref strategy in PageMvc
weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
- s/roteMatch/routeMatch/g
- s/getUseRouteMatch/useRouteMatch/g
weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants