Continuation - Since our CMS Drupal does occasionally assign the value zero to userID

[btruong]

This is a continuation of the closed PR #77 and applies suggestions from @dustincurrie in the comments of that closed PR.

[f2prateek]

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.

Why can't this be fixed upstream of the library? When is sending 0 as the userId valid?

[dustincurrie]

It can be fixed upstream.

My thought for this pull request is that analytics.js allows user 0. I think the PHP library should be expected to behave the same as the JS library.

[f2prateek]

SGTM - it sucks that 0 is allowed but I don't have a strong reason to disallow it other than that it might bite clients but there are other values that could be just as problematic.

cc @calvinfo and @dominicbarnes if I could get your eyes on this as well!

[calvinfo]

SGTM, thanks for the contribution @btruong. Do you mind adding a test case for the zero user and showing that it passes validation?

[btruong]

Thanks for you input @f2prateek and @calvinfo, I just updated the PR with some improved logic and we were able to test and verify on our Drupal instance that when the userId is zero, we no longer get an error using the following logic

!(array_key_exists('userID', $message) && strlen((string) $message['userID']) > 0)

this allows all empty checks to correctly detect when the $message array has an array key of 'userID' and whatever that value happens to be, it will be type-casted as a string so that we can use strlen to detect for the presence of 0 values.

The question remains, we have anonymous users throughout our Drupal site triggering segment events and so why haven't we seen the error as often?

We also are considering the hunch that the the error that spawned this initiative might represent and edge case where the browser might have some kind of ad blocking enabled where cookies are not stored (ajs_anonymous_id, etc.) and that might have caused the Segment.php script to rely on the userId check instead. Here was the initial error that spawned our investigation:

Exception: Uncaught exception 'Exception' with message 'Segment::track() requires userId or anonymousId' in /var/www/html/sites/all/libraries/segmentio/lib/Segment.php:142

Stack trace
…::assert called at /var/www/html/sites/all/libraries/segmentio/lib/ Segment.php (112) …validate called at /var/www/html/sites/all/libraries/segmentio/lib/ Segment.php (34) in Segment::track called at /var/www/html/sites/all/modules/custom/rm_analytics/rm_analytics.module (683) in _rm_analytics_newsletter_modal_form_submit called at /var/www/html/includes/form.inc (1519) in form_execute_handlers called at /var/www/html/includes/form.inc (903) in drupal_process_form called at /var/www/html/includes/ajax.inc (386) in ajax_form_callback called at /var/www/html/includes/menu.inc (527) in menu_execute_active_handler called at /var/www/html/index.php (21)

[f2prateek]

Sorry for the delay here! The CI issues are a separate problem we'll get fixed, but I'd like to see some other changes here before merging:

1. Extract the logic into a helper function so it doesn't have to be redeclared in 3 places.
2. Rebase against master.

Tracking internally in JIRA as well — https://segment.atlassian.net/browse/LIB-142.

[nd4p90x]

The changes are causing tests to fail, I believe this is due to the variable name change for userID. Please review and make necessary changes. Thank you.

[nd4p90x]

Issue #157 is replacing this request