The PHP convention has been to return FALSE on a failure and not 0. It seems like Drupal should do the same to be consistent. I also can't see any benefit to returning 0 instead of FALSE, and consistency is all the rage right now.

Patch will be submitted shortly.

CommentFileSizeAuthor
#10 batch_revert-270045-10.patch1.42 KByched
#7 drupal_270045.patch14.45 KBdrewish
#6 drupal_270045.patch247 bytesdrewish
#4 use_boolean_values.patch14.67 KBSusurrus
#2 use_boolean_values.patch13.39 KBSusurrus
#1 use_boolean_values.patch11.6 KBSusurrus
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Susurrus’s picture

Susurrus CreditAttribution: Susurrus commented
Status: Active » Needs review
FileSize
use_boolean_values.patch11.6 KB

Another case where booleans should've been used, but an int was used instead was wrapped into this patch.

Susurrus’s picture

Susurrus CreditAttribution: Susurrus commented
Title: Functions should return FALSE on failure, not 0 » Functions should use boolean values correctly
FileSize
use_boolean_values.patch13.39 KB

Caught a few functions returning preg_match[_all], which gives us an integer, so that was cast to a bool. Not sure what the code styles are for casting, however.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work

Please add the -p option to your cvs diff. I need to see the function names and that will do it for me. The construct of your cast looks similar to others I've seen within Drupal.

Susurrus’s picture

Susurrus CreditAttribution: Susurrus commented
Status: Needs work » Needs review
FileSize
use_boolean_values.patch14.67 KB

Here's an updated one to use the -p option. Eclipse doesn't allow for this option...

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Needs review » Needs work

no longer applies, three or four hunks fail

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Needs work » Needs review
FileSize
drupal_270045.patch247 bytes

my bad, it still applies. here's a clean re-roll.

drewish’s picture

drewish CreditAttribution: drewish commented
FileSize
drupal_270045.patch14.45 KB

i'll try that again.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Oh yes. This has bugged me for a long long time.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

yched’s picture

yched CreditAttribution: yched commented
Status: Fixed » Needs review
FileSize
batch_revert-270045-10.patch1.42 KB

Attached patch reverses the batch.inc changes : $finished is indeed a float completion level, used to provide progress info on multi-pass operations. (added a small comment to make it clearer)

The changes didn't break anything (because 1 == TRUE, 0 == FALSE), but are misleading.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Commited to CVS HEAD. Sorry, I should have spotted that.

keith.smith’s picture

keith.smith CreditAttribution: keith.smith commented

Putting in my tracker to add some periods after comments as a followup. Will adjust status when I have a patch.

keith.smith’s picture

keith.smith CreditAttribution: keith.smith commented

Ignore my comment #12; it was obviously late and I didn't see the second line of the comments which correctly contained full-stops.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.