Code improvements

Thanks @naveenvalecha for creating this issue, We will test your patch locally.

Hi @naveenvalecha thanks a lot for spending time on this.
After applying the patch I got the following message :

Checking patch README.txt...
Checking patch adobeanalytics.module...
Checking patch adobeanalytics.permissions.yml...
error: while searching for:
administer AdobeAnalytics configuration:
title: 'Configure Adobe Analytics settings'
description: 'Configure Adobe Analytics settings.'
#access to Configure the Adobeanalytics form is restricted to admin only.
restrict access: true

error: patch failed: adobeanalytics.permissions.yml:1
error: adobeanalytics.permissions.yml: patch does not apply
Checking patch composer.json...
Checking patch config/install/adobeanalytics.settings.yml...
Checking patch config/schema/adobeanalytics.schema.yml...
Checking patch src/AdobeAnalyticsHelper.php...
Checking patch src/Form/AdobeanalyticsAdminSettings.php...

Seems like the patch was not created properly or am I missing out on something?
It would be great if you can provide another patch for this.

Thank You :)

Hi @naveen

yes the repository is up to date

The o/p if git log is as follows

commit 203f6bb7b4798f5337ad261664b8f991b326e6ad
Author: kmoll
Date: Fri Dec 9 11:42:00 2016 -0500

Issue #2831117 by badjava, kmoll: Add composer.json file for module

commit d663e9991acc27621bcfd2d588ed268aa4be8523
Author: kmoll
Date: Fri Dec 9 10:40:33 2016 -0500

Issue #2700715 by kmoll, zeeshan_khan, heddn, panshulk, ARUN AK, Manjit.Sing

commit 8470a6f64566406e403d04d2db506c0d1aecd1b2
Author: Zeeshan Khan
Date: Mon Nov 28 14:47:03 2016 +0530

Issue #2700715: Removed define from module and used const instead and moved

commit b455a80d3f682bdf10d631988ba9b0e1687ca594
Author: Zeeshan Khan
Date: Mon Nov 28 14:43:53 2016 +0530

Issue #2700715: Removed define from module and used const instead.
:...skipping...
commit 203f6bb7b4798f5337ad261664b8f991b326e6ad
Author: kmoll
Date: Fri Dec 9 11:42:00 2016 -0500

Issue #2831117 by badjava, kmoll: Add composer.json file for module

commit d663e9991acc27621bcfd2d588ed268aa4be8523
Author: kmoll
Date: Fri Dec 9 10:40:33 2016 -0500

Issue #2700715 by kmoll, zeeshan_khan, heddn, panshulk, ARUN AK, Manjit.Sing

commit 8470a6f64566406e403d04d2db506c0d1aecd1b2
Author: Zeeshan Khan
Date: Mon Nov 28 14:47:03 2016 +0530

Hi @naveenvalecha , I have provided you with the commit access, Now you can make the desired code improvements.

I will review this patch today. @naveenvalecha, is there a specific reason the composer.json file was removed in the patch?

I took a look at the patch in #2 and think there are a number of things todo.

However these are really scraping the barrel for nit picks and I think this issue should be a blocker for anything.

1. +++ b/adobeanalytics.module
   @@ -63,15 +63,13 @@ function adobeanalytics_page_bottom(&$variables) {
   +        $config->get('codesnippet'), $context, array(
   

   As we are looking at really picky nit picks, I think we should move to short syntax arrays.

2. +++ /dev/null
   @@ -1,6 +0,0 @@
   -{
   -    "name": "drupal/adobeanalytics",
   -    "description": "Adds Adobe Analytics javascript tracking code to your site's pages.",
   -    "type": "drupal-module",
   -    "license": "GPL-2.0+"
   -}
   

   There's no harm in having a composer.json, if anything I'd advise it.

3. +++ b/src/Form/AdobeanalyticsAdminSettings.php
   @@ -80,8 +80,8 @@ class AdobeanalyticsAdminSettings extends ConfigFormBase {
   +    $default_value = ($config->get("role_tracking_type")) ? $config->get("role_tracking_type") : 'inclusive';
   

   I'm not sure this needs the extra brackets.

4. +++ b/src/Form/AdobeanalyticsAdminSettings.php
   @@ -91,17 +91,17 @@ class AdobeanalyticsAdminSettings extends ConfigFormBase {
   +      '#default_value' => empty($config_track_roles) ?
   +        array_keys($roles) : $config->get('track_roles'),
   

   This might be easier to read as a single line.

I have made a few small updates. In removing the `adobeanalytics` namespace from some of the form variables, it was not removed from the function to add the variable inputs, so it was not putting the variables in the table, and not saving the variables in the submit function.

I updated the patch and am committing. There is still an issue with the functionality of the variables form, but that is outlined in the issue #2831122: Add variable section of form does not work and should be updated shortly.

committing the updates. Thanks!

Thanks all for reporting this and picking this up :)