Skip to content

Since our CMS Drupal does occasionally assign the value zero to userID…#77

Closed
btruong wants to merge 1 commit intosegmentio:masterfrom
btruong:master
Closed

Since our CMS Drupal does occasionally assign the value zero to userID…#77
btruong wants to merge 1 commit intosegmentio:masterfrom
btruong:master

Conversation

@btruong
Copy link

@btruong btruong commented Sep 22, 2016

…, we need to allow segment calls to still validate when the userId is zero, so instead using not empty, using isset would allow the call to validate if the userId value happens to be zero

…, we need to allow segment calls to still validate when the userId is zero, so instead using not empty, using isset would allow the call to validate if the userId value happens to be zero
@f2prateek
Copy link
Contributor

f2prateek commented Oct 5, 2016

0 is indeed a valid userId as per our spec and it seems reasonable to not prevent users from using it unless our spec defines it as invalid.

On the other hand it might certainly bite people if they have a bug and send data for multiple users under the same ID.

@f2prateek
Copy link
Contributor

After thinking this through, I'd rather leave the empty check since that guards against an empty string while isset does not.

Can you fix this in your CMS upstream of the library?

@f2prateek f2prateek closed this Nov 4, 2016
@dustincurrie
Copy link

dustincurrie commented Nov 8, 2016

You can test for not empty while allowing zero in PHP with
!empty($message["userId"]) || $message["userId"] == '0'

With the == operator, you get type juggling, so this will work for the integer 0 and string '0'

Can this PR be updated and re-opened?

@f2prateek
Copy link
Contributor

@dustincurrie The 2nd part of my hesitation still stands #77 (comment).

On the other hand it might certainly bite people if they have a bug and send data for multiple users under the same ID.

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

Successfully merging this pull request may close these issues.

3 participants