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

rroblik’s picture

Issue summary: View changes
rroblik’s picture

Issue summary: View changes

Fix typo

rroblik’s picture

Issue summary: View changes
rroblik’s picture

(sorry for multiples update, Drupal is hard to use for issues...)

hass’s picture

Thanks 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?

if (!drupal_strlen($custom_var_value)) {

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... :-)

rroblik’s picture

Hum 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.module

@@ -205,18 +205,14 @@
   foreach (array('dimension', 'metric') as $googleanalytics_custom_type) {
       $googleanalytics_custom_vars = variable_get('googleanalytics_custom_' . $googleanalytics_custom_type, array());
       for ($i = 1; $i <= 20; $i++) {
-        $custom_var_value = !empty($googleanalytics_custom_vars['indexes'][$i]['value']) ? $googleanalytics_custom_vars['indexes'][$i]['value'] : '';
-        if (!empty($custom_var_value)) {
+        $custom_var_value = $googleanalytics_custom_vars['indexes'][$i]['value'];
+        if (drupal_strlen(trim($custom_var_value))) {
           $types = array();
           $node = menu_get_object();
           if (is_object($node)) {
             $types += array('node' => $node);
           }
           $custom_var_value = token_replace($custom_var_value, $types, array('clear' => TRUE));

googleanalytics.admin.inc

@@ -328,7 +328,7 @@
       '#type' => 'textfield',
     );
     $form['googleanalytics_custom_dimension']['indexes'][$i]['value'] = array(
-      '#default_value' => !empty($googleanalytics_custom_dimension['indexes'][$i]['value']) ? $googleanalytics_custom_dimension['indexes'][$i]['value'] : '',
+      '#default_value' =>  drupal_strlen($googleanalytics_custom_dimension['indexes'][$i]['value']) ? $googleanalytics_custom_dimension['indexes'][$i]['value'] : '',
       '#description' => t('The custom dimension value.'),
       '#maxlength' => 255,
       '#title' => t('Custom dimension value #@index', array('@index' => $i)),
@@ -377,7 +377,7 @@
       '#type' => 'textfield',
     );
     $form['googleanalytics_custom_metric']['indexes'][$i]['value'] = array(
-      '#default_value' => !empty($googleanalytics_custom_metric['indexes'][$i]['value']) ? $googleanalytics_custom_metric['indexes'][$i]['value'] : '',
+      '#default_value' => drupal_strlen($googleanalytics_custom_metric['indexes'][$i]['value']) ? $googleanalytics_custom_metric['indexes'][$i]['value'] : '',
       '#description' => t('The custom metric value.'),
       '#maxlength' => 255,
       '#title' => t('Custom metric value #@index', array('@index' => $i)),

Should we expect to see it in new minor stable version (eg 2.1) ?

Thanks

hass’s picture

Version: 7.x-2.0 » 8.x-2.x-dev
Status: Active » Needs review
StatusFileSize
new2.84 KB
new3.79 KB

Let's see if the new tests are failing.

hass’s picture

Version: 8.x-2.x-dev » 7.x-2.x-dev
StatusFileSize
new2.7 KB
new3.66 KB

The last submitted patch, 8: Issue 2300701-Test-only-D7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: Issue-2300701-by-hass-Custom-dimensions-and-custom-m-D7.patch, failed testing.

hass’s picture

hass’s picture

Version: 7.x-2.x-dev » 8.x-2.x-dev
StatusFileSize
new5.29 KB
hass’s picture

Status: Needs review » Needs work
hass’s picture

hass’s picture

  • hass committed a9cbec5 on 7.x-2.x
    Issue #2300701: Custom dimensions and custom metrics not outputed on...

  • hass committed 7893ea8 on 8.x-2.x
    Issue #2300701 by hass: Custom dimensions and custom metrics not...
hass’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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

The last submitted patch, 7: Issue-2300701-Custom-dimensions-and-custom-metrics-n.patch, failed testing.

Status: Closed (fixed) » Needs work
hass’s picture

Status: Needs work » Closed (fixed)

The last submitted patch, 7: Issue-2300701-Tests-only.patch, failed testing.