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

prepend basePath when autogenerating from route result #15

Merged
merged 2 commits into from
Aug 20, 2016

Conversation

pine3ree
Copy link
Contributor

@pine3ree pine3ree commented Jun 1, 2016

I believe that invoking helper should end in consistent results with generated urls containing - if set - the basePath prefix.
Should we also avoid getter (getRouteResult, getBasePath) calls and use $this->result and $this->basePath directly?
(code contained mixed getter and property call for result)
I don't see any reason why those getter should be eventually overridden in a child class to send something different from the internal properties themselves.

kind regards

I believe that invoking helper should end in consistent results with generated urls containing - if set - the basePath prefix.
Should we also avoid getter (getRouteResult, getBasePath) calls and use $this->result and $this->basePath directly? 
(code contained mixed getter and property call for result)
I don't see any reason why those getter should be eventually overridden in a child class to send something different from the internal properties themselves.

kind regards
@pine3ree
Copy link
Contributor Author

pine3ree commented Jun 1, 2016

One difference from: #11
i believe we should use basePath only when invoking the helper and not when calling the private method generateUriFromResult. If we strip the basePath from the uri for a website running in a subdir, we then set the base path in the generator, but for the router and route result that basePath does not exist.

@michaelmoussa
Copy link
Contributor

@pine3ree this makes sense. Thanks! Can you add some tests for the affected cases? The test coverage still happens to be 100% after your changes, but there are no test cases to cover specifically that the basePath is prepended when the provided $route is null.

@pine3ree
Copy link
Contributor Author

@michaelmoussa
Hello, i added a simple test case. Pls let me now if this would suffice. kind regards.
maks

@michaelmoussa michaelmoussa merged commit d0fb64a into zendframework:master Aug 20, 2016
michaelmoussa added a commit that referenced this pull request Aug 20, 2016
michaelmoussa added a commit that referenced this pull request Aug 20, 2016
michaelmoussa added a commit that referenced this pull request Aug 20, 2016
@pine3ree pine3ree deleted the patch-2 branch August 20, 2016 20:03
@michaelmoussa
Copy link
Contributor

@pine3ree this is fixed in 2.0.2.

I made a minor adjustment in addition to your commit to remove the need for the else / if. The way it looks now is similar to in the previous version, but with the $basePath still being prepended always.

Thanks for your contribution!

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

Successfully merging this pull request may close these issues.

2 participants