search

symfony-cmf / Routing

Routing component building on the Symfony Routing component
Other
288 stars 69 forks source link

Rename event property from name to route to avoid conflicts #135

Closed luishdez closed 9 years ago

luishdez commented 9 years ago

This patch fix https://github.com/symfony-cmf/Routing/issues/134

 Q A
Bug fix? yes
New feature?  no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets https://github.com/symfony-cmf/Routing/issues/134
License MIT
Doc PR -
dbu commented 9 years ago

thanks for the pull request. i agree with the changes on the event, but not the changes on the DynamicRouter.

@benglass are you already using this feature? if so, you should watch this issue and when we merge, update your code. the nasty thing about it is you will get no error. but setName is a general thing of events, you can change what the event is called. calling setName instead of setRoute will simply have no effect anymore...

dbu commented 9 years ago

we don't need to adjust the documentation really, as https://github.com/symfony-cmf/symfony-cmf-docs/pull/672 is not yet merged.

benglass commented 9 years ago

@dbu we are not using this yet but thanks very much for the heads up

wouterj commented 9 years ago

This should be mentioned as a BCbreak in the changelog

dbu commented 9 years ago

@WouterJ it was added 2 weeks ago and never released. but to be on the safe side we could still add it to CHANGELOG.md, yes

luishdez commented 9 years ago

Ops I missed that RouterInterface.

Regarding the Changelog if you guys are talking about this method rename… I don't see the point. It was broken, so nobody has been able to use it anyways…???

Looks that something is failing here https://travis-ci.org/symfony-cmf/Routing/builds/59834355 but I'm not able to reproduce it…

unittest

wouterj commented 9 years ago

@luishdez those tests are based on timing and can randomly fail. I've restarted them now and they pass.

dbu commented 9 years ago

thanks a lot @luishdez !

@WouterJ i concur with his analysis that this could not have worked in real life (it worked only in the situation of the test cases i think)