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

AMQPMessage::basic_consume + $nowait=null results in $nowait=true #422

Closed
jamarr81 opened this issue Jul 13, 2016 · 4 comments
Closed

AMQPMessage::basic_consume + $nowait=null results in $nowait=true #422

jamarr81 opened this issue Jul 13, 2016 · 4 comments
Assignees
Milestone

Comments

@jamarr81
Copy link

This is a minor, but annoying, bug. Basically, if $nowait=null is given to AMQPMessage::basic_consume, it's treated as if $nowait=true. It should be treated as $nowait=false.

The problem is that basic_consume uses the identity operator (===) instead of the equality operator (==) when checking $nowait on line 397.

@michaelklishin
Copy link
Collaborator

Why do you think so? It's not unreasonable for a library to default to true. I'm hardly a PHP expert but as a maintainer of many client libraries I'd certainly treat null as "use a default value", not necessarily false.

@jamarr81
Copy link
Author

jamarr81 commented Jul 14, 2016

In PHP it is an unwritten standard that null values always be treated as falsy when checking for a boolean result; mainly because PHP specifies this definition via type juggling semantics.

That said, the identity (===) operator's main use is to determine if two objects are in fact the same instance of a class...where as the equality (==) operator's main use is to determine if two objects or values are equivalent (but not necessarily the same 'instance').

Finally, in this specific case, the default parameter value for $nowait (in AMQPMessage::basic_consume) is false...so even if we ignore all of the above, it still makes sense for null to be equated as false using your logic ;)

@hairyhum
Copy link
Collaborator

The function API suggests you use a boolean value, not null or even an object (which could be set to null). Implicit type conversions in PHP are hard enough to add additional complexity by two falsy values explicitly in the library code.
Instead, for my opinion, type check should throw an exception if an invalid parameter value is supplied.

@lukebakken lukebakken added this to the 2.7.1 milestone Jan 16, 2018
@lukebakken lukebakken assigned lukebakken and unassigned lukebakken Jan 16, 2018
@lukebakken lukebakken modified the milestones: 2.7.1, 2.7.2 Jan 29, 2018
@lukebakken lukebakken removed this from the 2.7.2 milestone Feb 10, 2018
@lukebakken lukebakken added this to the 3.x.x milestone Feb 27, 2019
@lukebakken lukebakken self-assigned this Feb 27, 2019
@ramunasd
Copy link
Member

$nowait must be boolean, either true or false. In next major release we will add typehints and any non boolean argument just wont work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants