First of all, thanks for the hard work done for this new 2.0 version, working like a charm :)
I found a little bug in gooogleanalytics.module, related to custom metric and dimension (here cmd)
If you put zero value inside cmd, the value is not outputed client side by javascript. I think it's a bug because we can send zéro "0" value to GA to act, for example, as a boolean.
The reason is at line 208
$custom_var_value = !empty($googleanalytics_custom_vars['indexes'][$i]['value']) ? $googleanalytics_custom_vars['indexes'][$i]['value'] : '';
if (!empty($custom_var_value)) {
...
The check with empty is bad, because empty(0) and empty('0') return false, so if we put "0" inside cdm or via php (variable override) the test not succeed in and the variable is not outputted.
Fix :
googleanalytics.module
// line 208
$custom_var_value = $googleanalytics_custom_vars['indexes'][$i]['value']; // As this is set by module, no need to check, no warning possible
if (!empty($custom_var_value) || (empty($custom_var_value) && is_numeric($custom_var_value))) {
...
But the same bad check is done in googleanalytics.admin.inc line 331 for dimension, line 380 for metric ... So maybe the simplest and cleanest way to fix it is to add a function inside googleanalytics.module
Fix : (cleanest solution)
googleanalytics.module
/**
* Check is variable is empty or
* 0 (0 as an integer)
* 0.0 (0 as a float)
* "0" (0 as a string)
*
* @param mixed $value
* @return boolean
*/
function _googleanalytics_is_blank($value){
return empty($value) && !is_numeric($value);
}
Then
// line 208
$custom_var_value = $googleanalytics_custom_vars['indexes'][$i]['value']; // As this is set by module, no need to check, no warning possible
if (!_googleanalytics_is_blank($custom_var_value)) {
...
googleanalytics.admin.inc
// line 330
$form['googleanalytics_custom_dimension']['indexes'][$i]['value'] = array(
'#default_value' => !_googleanalytics_is_blank($googleanalytics_custom_dimension['indexes'][$i]['value']) ? $googleanalytics_custom_dimension['indexes'][$i]['value'] : '',
...
// line 380
$form['googleanalytics_custom_metric']['indexes'][$i]['value'] = array(
'#default_value' => !_googleanalytics_is_blank($googleanalytics_custom_metric['indexes'][$i]['value']) ? $googleanalytics_custom_metric['indexes'][$i]['value'] : '',
...
What do you think about it ?
Thanks
Comments
Comment #1
rroblik commentedComment #2
rroblik commentedFix typo
Comment #3
rroblik commentedComment #4
rroblik commented(sorry for multiples update, Drupal is hard to use for issues...)
Comment #5
hass commentedThanks for the code examples... I think the intention was only to not run the code if it's an empty string, and yes empty is bad idea here. How about strlen() check?
Less complex, less checks and same result. We need to add a test for this, so this does not happen again. Aside - this bug exists for many years and was also inside custom variables... :-)
Comment #6
rroblik commentedHum yeah I didn't know all about this Drupal "magic" functions, effectively lightweight and don't need to add custom function :)
After tests, all working like charm :)
Here are (new) cleanest changes, from last stable version files :
googleanalytics.modulegoogleanalytics.admin.incShould we expect to see it in new minor stable version (eg 2.1) ?
Thanks
Comment #7
hass commentedLet's see if the new tests are failing.
Comment #8
hass commentedComment #11
hass commentedComment #12
hass commentedComment #13
hass commentedComment #14
hass commentedComment #20
hass commentedComment #22
hass commentedComment #25
hass commentedComment #31
hass commented