Misnamed variable fix. _compiledRoute now with a leading underscore #8396

Merged
merged 2 commits into from Mar 3, 2016

Projects

None yet

4 participants

@Phillaf
Contributor
Phillaf commented Mar 3, 2016

No description provided.

@Phillaf Phillaf Misnamed variable fix. _compiledRoute now with a leading underscore
e546162
@chinpei215 chinpei215 added this to the 3.2.4 milestone Mar 3, 2016
@chinpei215 chinpei215 referenced this pull request Mar 3, 2016
Closed

RFC: Stop using empty() as much as possible? #8397

1 of 3 tasks complete
@dereuromark dereuromark and 1 other commented on an outdated diff Mar 3, 2016
src/Routing/Route/Route.php
@@ -146,7 +146,7 @@ public function compiled()
*/
public function compile()
{
- if (!empty($this->compiledRoute)) {
+ if (!empty($this->_compiledRoute)) {
@dereuromark
dereuromark Mar 3, 2016 Member

if ($this->_compiledRoute) { then, as we know this property must exist.

@chinpei215
chinpei215 Mar 3, 2016 Member

Yes. But at the moment, I have no objection to use empty() against an existing property :)
Thank you for finding this issue, @Phillaf.

@dereuromark
dereuromark Mar 3, 2016 Member

@chinpei215 You pointed out a valid flaw in the current way of checking. Currently using !empty() is quite dangerous as it would false negative once the variable changes or there is a typo without anyone noticing (assuming there is a missing test case for it).
So it would be sane to start doing this in a way that existing properties will cause an error if you (re)name them wrongly. Functionally both behave 100% the same thanks to PHP.

@dereuromark dereuromark added the Defect label Mar 3, 2016
@Phillaf Phillaf refs #8397
10bee08
@chinpei215
Member

@dereuromark I totally agree with you. Simply, I feel sorry to @Phillaf because he sent this PR before my suggestion. But he has already fixed it :D Thanks again, @Phillaf!

@markstory markstory merged commit 0e98459 into cakephp:master Mar 3, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Scrutinizer No new issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@markstory
Member

Thanks @Phillaf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment