filter routes with the id prefix before doing lookups #186
Conversation
lsmith77
commented
Oct 7, 2013
lsmith77
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #185 |
| License | MIT |
| Doc PR | n/a |
|
+1, looks good to me. i think this bundle had pretty much 100% code coverage - we probably miss a test. the existing tests reveal that there are missing sanity checks - see travis. i think there is a edge case BC break: if somebody relied previously that he would get routes even if they where stored outside the idprefix of this provider. if there are several roots and providers and listeners, things would until now have accidentally worked but will no longer. maybe we should add a note in the CHANGELOG to aovid headaches over that in case somebody is doing this. |
| @@ -120,9 +120,11 @@ protected function getCandidates($url) | |||
| public function getRouteByName($name, $parameters = array()) | |||
| { | |||
| // $name is the route document path | |||
| $route = $this->getObjectManager()->find($this->className, $name); | |||
| if ($this->idPrefix && 0 === strpos($name, $this->idPrefix)) { | |||
There was a problem hiding this comment.
is it sane that the idPrefix could be null? if its "" then we should stll try to load $name
i think the name could be null or empty however...
There was a problem hiding this comment.
indeed .. it should be if (!$this->idPrefix || 0 === strpos($name, $this->idPrefix)) {
There was a problem hiding this comment.
@cdwlighting can you have a look here?
|
i won't merge this with tests failing. but i think it just needs some more sanity checking and then should be good. if it does not make it into 1.1.0 i still think we could consider it a non-BC-breaking performance fix that can go into the 1.1 branch |
Fixed getRoutesByName to work if $idPrefix is not defined. Added Tests to cover those cases.
…es the foreach loop. Removed the .idea file from .gitignore Added Back the missing getRoutesByNames skipped test.
Created Unit tests for getRoutesByName with defined idPrefix.
filter routes with the id prefix before doing lookups
|
thanks a lot @lsmith77 and @cdwlighting ! |
