Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Presenter::argsToParams to not remove false #107

Closed
wants to merge 1 commit into from

Conversation

@enumag
Copy link
Contributor

enumag commented Dec 15, 2015

I sometimes use boolean parameters in presenters and I want the param=0 part in the URL because FALSE and NULL are totally different values for me. Right now however argsToParams replaces false values with nulls which breaks my use case.

@matej21

This comment has been minimized.

Copy link
Contributor

matej21 commented Dec 15, 2015

👍

@@ -1047,7 +1047,7 @@ public static function argsToParams($class, $method, & $args, $supplemental = []
));
}

if ($args[$name] === $def || ($def === NULL && is_scalar($args[$name]) && (string) $args[$name] === '')) {
if ($args[$name] === $def || ($def === NULL && is_scalar($args[$name]) && !is_bool($args[$name]) && (string) $args[$name] === '')) {

This comment has been minimized.

Copy link
@matej21

matej21 Dec 15, 2015

Contributor

I think, is_scalar($args[$name]) && !is_bool($args[$name]) && (string) $args[$name] === '' can be simplified to $args[$name] === '' :)

This comment has been minimized.

Copy link
@enumag

enumag Dec 15, 2015

Author Contributor

Probably. Integers and floats should never become an empty string after type casting.

@enumag enumag force-pushed the enumag:patch-8 branch from 0433169 to 6f1b61e Dec 15, 2015
@enumag enumag force-pushed the enumag:patch-8 branch from 6f1b61e to 50c9a06 Dec 15, 2015
@dg dg closed this in 2960301 Jan 13, 2016
@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Jan 13, 2016

@dg Thanks!

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 13, 2016

thanks to you ;)

dg added a commit that referenced this pull request Jan 13, 2016
@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 13, 2016

The question is whether to include it in v2.3…

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Jan 13, 2016

It admitedly is a BC break so as much as I'd like it in stable 2.x, putting it in 2.3 is probably not a best idea.

Are there other commits like this? If so we might want to release nette/application 2.4.

@dg

This comment has been minimized.

Copy link
Member

dg commented Jan 13, 2016

I did not realize that it is a BC break, so it will be in 2.4.

@@ -1047,7 +1047,7 @@ public static function argsToParams($class, $method, & $args, $supplemental = []
));
}

if ($args[$name] === $def || ($def === NULL && is_scalar($args[$name]) && (string) $args[$name] === '')) {

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Jan 13, 2016

Contributor

Just wondering, why did you remove is_scalar here?

This comment has been minimized.

Copy link
@enumag

enumag Jan 13, 2016

Author Contributor

Because as far as I can tell is_scalar($args[$name]) && (string) $args[$name] === '' is equivalent to $args[$name] === '' || $args[$name] === false.

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Jan 13, 2016

Well since it required a change in tests and generates different URLs in some cases I would consider it a BC break...

dg added a commit that referenced this pull request Jan 13, 2016
…L and FALSE [Closes #107]

# Conflicts:
#	tests/UI/Presenter.link().php7.phpt
dg added a commit that referenced this pull request Jan 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.