Update WizardComponent.php

[mlingureanu]

test type for $proceed

[ProLoser]

I'll leave this up to @bancer who's become far more active on this repo than I. Also, thank you @mlingureanu for your first contribution!

[ProLoser]

@mlingureanu since this is your first contribution, can I ask you what this PR is trying to accomplish? What was the problem you ran into that made this fix necessary?

[mlingureanu]

The documentation says

The process callback must return either true or false. If true, the wizard will continue to the next step; if false, the user will remain on the step and any validation errors will be presented.

I run into trouble, on unit tests, when instead of returning boolean, I need to force a redirect like
return $this->redirect($url);
The problem is, this process callback will return an mocked object in unit tests, and variable $proceed from wizard component will get this mocked object instead of boolean value.
Next condition if ($proceed) without checking type, will assume this object is true and will execute unwanted code.

[ProLoser]

Why are you getting an object? Sounds like a bug in your tests.

…

On Fri, Mar 31, 2017 at 10:27 PM Manuel Lingureanu ***@***.***> wrote: The documentation says The process callback must return either true or false. If true, the wizard will continue to the next step; if false, the user will remain on the step and any validation errors will be presented. I run into trouble, on unit tests, when instead of returning boolean, I need to force a redirect like return $this->redirect($url); The problem is, this process callback will return an mocked object in unit tests, and variable $proceed from wizard component will get this mocked object instead of boolean value. Next condition if ($proceed) without checking type, will assume this object is true and will execute unwanted code. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEHQ39g5lZ2xdiYkZFj4yuDwybc7txRks5rreBJgaJpZM4MwI2x> .

[mlingureanu]

When testing actions that contain redirect() and other code following the redirect it is generally a good idea to return when redirecting. The reason for this, is that redirect() is mocked in testing, and does not exit like normal. And instead of your code exiting, it will continue to run code following the redirect.

https://book.cakephp.org/2.0/en/development/testing.html#testing-controllers

[bancer]

I am not convinced this change is needed while the same can be achieved by $this->redirect($url); return false; or $this->redirect($url); return true; in the controller. @ProLoser do you have an opinion on that?

[ProLoser]

I'm too removed from the code to know how it works anymore, I just don't like strict comparisons to true unless you handle other truthy values differently (which this code doesn't)

[mlingureanu]

$proceed from line 379 and line 381 should return always boolean, that's why it's a matter of best practices, to test type when dealing with booleans

[mlingureanu]

https://phpbestpractices.org/#checking-for-null

[mlingureanu]

Gotchas
When testing the return value of a function that can return either 0 or boolean false, like strpos(), always use === and !==, or you’ll run in to problems.

[ProLoser]

That specifically talks about falsey values if they represent different things than null.

Again, I think something is wrong with your unit test. I need more context on how this code is called.

[bancer]

Looks good for me. @ProLoser ?

[ProLoser]

Why does processCallback not return a boolean?

[mlingureanu]

in WizardComponentTest.php at line 119

public function redirect($url = null, $status = null, $exit = true) {
// Do not allow redirect() to exit in unit tests.
return parent::redirect($url, $status, false);
}

it's override default cakephp redirect method to not exit and to continue to how Bancer says

$this->redirect($url); return false; or $this->redirect($url); return true

this is a snippet from my code:

public function processRestaurant() {
		if (!empty($this->request->data)) {
		...
		// Create the redirect url with the new restaurant and the extra values from before.
		$url = array(
			'controller' => 'book',
			'action' => 'table',
			$stepToGo,
			$restaurantId,
		);
		$url = array_merge($url, $extra);
		$this->redirect($url, null, false);
		return false;
	}
	return false;
}

I need this redirect, because I need to change param $restaurantId and method redirect from wizard don't allow params to be changed.

[mlingureanu]

by the way this comment // Do not allow redirect() to exit in unit tests. it's wrong because in unit tests redirect is mocked and third param $exit is false

[ProLoser]

I mean I'll leave it to bancer