Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Seed RouteMatch params as long as params is set. This permits setting an empty array. #2285

Merged
merged 1 commit into from Sep 10, 2012

Conversation

Projects
None yet
5 participants
Contributor

Bilge commented Sep 1, 2012

No description provided.

This pull request passes (merged d465123 into 3a4cee6).

@Freeaqingme Freeaqingme commented on the diff Sep 2, 2012

library/Zend/Mvc/Controller/Plugin/Forward.php
@@ -136,7 +136,7 @@ public function dispatch($name, array $params = null)
}
// Allow passing parameters to seed the RouteMatch with
- if ($params) {
+ if (isset($params)) {
@Freeaqingme

Freeaqingme Sep 2, 2012

Member

Since params is always set, perhaps it would make more sense to simply check if it's not null? But then again, that is already what this if-clause was doing already, so I am kinda missing the significance of this PR. Perhaps you can elaborate?

@Bilge

Bilge Sep 2, 2012

Contributor

It is not the same. If you pass an empty array the old check will ignore it; isset() treats null and an empty array differently.

@DASPRiD

DASPRiD Sep 3, 2012

Member

I'd argue with @Freeaqingme here, that it should likely be ($params !== null).

@Bilge

Bilge Sep 3, 2012

Contributor

What exactly do you think isset() does?

@DASPRiD

DASPRiD Sep 3, 2012

Member

isset() primarily checks if a variable actually exists. Only secondarily it checks if it is not null. Using an implicit isset() check yields the option that $params could not have been set in the method.

@weierophinney weierophinney added a commit that referenced this pull request Sep 10, 2012

@weierophinney weierophinney [#2285] Wrote tests
- Provided tests to ensure you can pass an empty array for route matches
  when using the forward() plugin.
- Changed test from isset() to '!== null'
6b3602f

@weierophinney weierophinney merged commit d465123 into zendframework:master Sep 10, 2012

1 check passed

default The Travis build passed
Details
Owner

weierophinney commented Sep 10, 2012

Agreed with @DASPRiD and @Freeaqingme; using "isset()" will test for a non null, non-zero, non empty string, non empty array value. Since the typehint is for array, the only possible values are null or an array -- so testing for not null is more explicit and semantically correct.

I added tests to verify the change, and merged to release and master branches.

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