[D8]AdobeAnalytics

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxpanshulk12641758git

Fixed the git clone URL in the issue summary for non-maintainer users.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Full manual project review :
- Can you make AdobeAnalyticsHelper.php a Controller. Any custom code in a module can be placed as a Controller for better usage
- README.txt needs a D8 touch. The code examples mentioned are still in D7 code
- there is no default value for adobeanalytics_track_roles: variable in adobeanalytics.settings.yml
- Please correct '#open' => TURE in AdobeanalyticsAdminSettings.php
- adobeAnalyticsTokenReplace() - this function does not has any return value in AdobeAnalyticsHelper.php
- adobeAnalyticsGetTokenContext() - this function does not return any value and its return value being used on line 51 in AdobeAnalyticsHelper.php $context = !empty($variables) ? $this->adobeAnalyticsGetTokenContext() : array();

One important thing
Can you please help us in the reviewing process and review at least 3 new projects from the issue queue. After reviewing 3 projects, you can add PAReview: review bonus tag.

Manual Code review:

1. There are lot of unnecessary temporary files are there. Can you please remove it from the repo.
2. What is the .idea directory ? Please remove it.
3. description: 'Adds Adobe Analytics javascript tracking code to your site''s pages.'
   Please correct the description in adobeanalytics.info.yml
4. Correct the function adobeanalytics_help description. I guess it should be return t("Settings for Adobe analytics");

And Please add review bonusof your latest review of other project applications. This will speedup the review process of your application.

Functionality issues:

• No success message on saving configuration.
• Improvement - JS scope needs to be added in config form if files are going to be added on top of the page or bottom of the page.

Saving conf variables can be handled with good better way.

$config->set('adobeanalytics_extra_variables', $extra_vars)->save();
    $config->set('adobeanalytics_js_file_location', $form_state->getValue('adobeanalytics_js_file_location'))->save();
    $config->set('adobeanalytics_image_file_location', $form_state->getValue('adobeanalytics_image_file_location'))->save();
    $config->set('adobeanalytics_version', $form_state->getValue('adobeanalytics_version'))->save();
    $config->set('adobeanalytics_token_cache_lifetime', $form_state->getValue('adobeanalytics_token_cache_lifetime'))->save();
    $config->set('adobeanalytics_codesnippet', $form_state->getValue('adobeanalytics_codesnippet'))->save();
    $config->set('adobeanalytics_role_tracking_type', $form_state->getValue('adobeanalytics_role_tracking_type'))->save();
    $config->set('adobeanalytics_track_roles', $form_state->getValue('adobeanalytics_track_roles'))->save();

Example like (#1910694: system_settings_form() is replaced by \Drupal\Core\Form\ConfigFormBase):

/**
   * {@inheritdoc}
   */
  public function submitForm(array &$form, array &$form_state) {
    $this->config('system.maintenance')
      ->set('enabled', $form_state['values']['maintenance_mode'])
      ->set('message', $form_state['values']['maintenance_mode_message'])
      ->save();

    parent::submitForm($form, $form_state);
  }

// $query_string = '?' . variable_get('css_js_query_string', '0');.

Remove commented code and debug code.

Proper documentation missing

/**
* Get the extra variable form with some conditions.
*/

Above mentioned small fixes are remaining. Please fix ASAP. module is ready to be RTBC after that.

Awesome - Reviewed the commit http://cgit.drupalcode.org/sandbox-panshulk1-2641758/commit/?id=9348fa5

Making this RTBC.

Manual Code review:
1. In template_preprocess_html(), the javascript is been embedded,
It would be great if this would be added in better way.
2. In admin/config/system/adobeanalytics, fields are taking whatever the input is given, IMO if validation is been implemented that would be great.

@panshulk
Please review at least 3 other projects and feel free to mark it as Needs Review and adding a PAReview: review bonus issue tag to speed up the process.
Moving the status back to Needs Work for now.

@gauravjeet: please only put applications to "needs work" if you did a code review and found blocking issues.

Please add review bonus tag when will you are adding your manual reviews.

Moving it to RTBC (as per #13 and #16) as there is nothing that would be blocker for you application.

project page is empty, please describe your module at https://www.drupal.org/sandbox/panshulk1/2641758 , see https://www.drupal.org/node/997024

manual review:

1. adobeanalytics.permissions.yml: why is the restrict access flag set for this permission? Please add a comment in the YAML.
2. AdobeAnalyticsHelper: the function menu_load() does not exist in Drupal 8.
3. SafeMarkup is deprecated and should not be used, see https://www.drupal.org/node/2549395
4. adobeanalytics_preprocess_html(): there is a var_dump() call that prints debugging info uncontrolled?
5. AdobeAnalyticsHelper: do not call \Drupal in classes, use dependency injection instead. See https://www.drupal.org/node/2133171 . It looks like AdobeAnalyticsHelper should be a service defined in a *.services.yml file.
6. do not use adobeanalytics_preprocess_html() since you are not processing the HTML, you just want to add stuff. Use hook_page_attachments() or hook_page_top() or hook_page_bottom() instead.
7. adobeanalytics_preprocess_html(): $query_string is always empty and can be removed.

The var_dump() is a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects. Please add them to the issue summary.

Hi,

I noticed the use of dependency injection and adding AdobeAnalyticsHelper as a service, suggested by @Klausi in comment #26 (item 5), was missing.
I've implemented those changes; plus a few other PHP notice fixes on the generation of the header and footer javascript code.

Please review.

Regards,

Hi,

I've been testing the module a little more; and I run into a possible issue. I saw something similar when I first checked the module, and included a few changes in my previous patch to fix that. But it now seems like this is something intentional. I wonder if this is by design, or if i'm just missing something.

I'm referring to the way some JS code gets printed on the page. In adobeanalytics.module, there's some code like this:

$js_file_location = ['#plain_text' => $adobeanalytics_js_file_location];
...
$header .= $js_file_location;

That would print something like this:

<script type="text/javascript" src="Array"></script>

And I see the same pattern (['#plain_text' => $some_js_code]) several times; like for instance when adding Adobe Analytics custom variables. The code is in AdobeAnalyticsHelper.php:

$key = ['#plain_text' => $key];
$value = $this->adobeAnalyticsTokenReplace($value);
$variables_formatted .= "{$key}=\"{$value}\";\n";

Again, that will attempt to print an array instead of the custom variable name ($key).
and the following Notice shows up:

Notice: Array to string conversion in Drupal\adobeanalytics\AdobeAnalyticsHelper->adobeAnalyticsFormatVariables() (line 85 of modules/contrib/adobeanalytics/src/AdobeAnalyticsHelper.php)

Please advice all this is by design, and how to print the correct JS code; or if this is in fact a bug that needs to be fixed.

Regards,

Still the following issues mentioned in #26 is not fixed. Please fix these issues.

do not use adobeanalytics_preprocess_html() since you are not processing the HTML, you just want to add stuff. Use hook_page_attachments() or hook_page_top() or hook_page_bottom() instead.

AdobeAnalyticsHelper: do not call \Drupal in classes, use dependency injection instead. See https://www.drupal.org/node/2133171 . It looks like AdobeAnalyticsHelper should be a service defined in a *.services.yml file.

Thanks,
ARUN AK

Review of the 8.x-1.x branch (commit bbd29de):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives. See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. "\Drupal::moduleHandler()->invokeAll('adobeanalytics_variables')": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
2. adobeanalytics_preprocess_html(): I think "new Markup()" is wrong here, you can just pass the strings into #markup of a render array?

And you should also fix the open points from above. Otherwise I think there are not critical application blockers left.

Assigning to Naveen as he might have time to take a final look at this.

Manual Review :

1. define('ADOBEANALYTICS_TOKEN_CACHE', 'adobeanalytics:tag_token_results'); Use const instead of define as it is two times faster than define.
2. adobeanalytics_help : Return an render array to get the cachebility metadata associated with it. See https://github.com/drupal/drupal/blob/8.2.x/core/modules/help/help.modul... Note this change was introduced in 8.1.x as 8.0.x is not being supported so use it.
3. (*) context_get_plugin : Where this method is defined ? This will break the site which will use the module
4. Define the schema/metadata of the config variables being used in the module.
5. AdobeanalyticsAdminSettings.php : wrong return type on getEditableConfigNames function.
6. Remove the comment // Fieldset changed to details in drupal 8. from AdobeanalyticsAdminSettings.php fieldsets are still there in d8 as well.
7. AdobeanalyticsAdminSettings.php : Use the injectable t method and use the single quotes over doubel quotes where possible.
8. (*)AdobeAnalyticsHelper.php : adobeAnalyticsGetTokenContext Instead of drupal_static define the property to a class and save the calculated context to it.
9. Coding standards are missing from lot of places. Fix them.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

I've created an issue on the sandbox to address some of these issues and get the conversation going again on getting this module updated, #2825875: Fix issues outlined on port issue.

I have taken the patch in #32 and updated it based on other feedback given here. There are still some issues with the module, but I feel we've removed the blockers for making this a project. If others feel that is the case, lets commit and make this a project and we can create issues to tackle the remaining problems.

I moved the patch over to #2700715: D8 Port of Adobe Analytics (SiteCatalyst) to D8 since that is an issue on the sandbox project. Once that is reviewed and commited, we can look for the project review to happen again.

Moving to RTBC as per https://www.drupal.org/node/2700715 fixes.

RTBC ++

Back to N/W there were lot of blockers and improvements that I found. I have done that in the issue #2839812: Code improvements After that its good to go.

There are some good suggestions in #2839812: Code improvements but I don't see any of them as a blocker for this being a full project. I'd be happy to see an alpha release with the current code, then follow ups to address these items before a beta release.

Awesome !! Thanks for working on it @Tim @Naveen

Assigning to myself. will review it tonight. The module has existing bug #2831122: Add variable section of form does not work

// Naveen

Yes @naveenvalecha. There are still some bugs here that we are continuously working on. I don't believe the module needs to have a stable release for it to be accepted and created as a project, which is what this ticket is for I believe. I don't seen any blockers or any reason why the sandbox should not be promoted as a full project as we continue to work on bugs and get it to a stable release. There will only be a 8.x-1.x-dev version at first, but then we will release alpha/beta releases as we fix these issues.

@kmoll,
I assigned myself for the second git admin review to review the code again b/c there were a couple of changes in the code from the last time when I review it. I'll be able to approve it after reviewing the code. See the process https://www.drupal.org/node/539608

you unassigned the issue from me, Do you want to fix the bugs first and then want to take the review ?

// Naveen

@Naveen, no the unassigned was unintentional, please assign it back to yourself and review when you have a moment. Thanks.

Review of the 8.x-1.x branch (commit de54a8c):

Pareview.sh : https://pareview.sh/node/735

1. adobeanalytics.permissions.yml : Permission key should be in lowercase administer AdobeAnalytics configuration:
2. Wrong docblock at adobeanalytics_page_bottom.
3. Typo in adobeanalytics.schema.yml
4. Define the Config schema type for the config keys token_cache_lifetime, extra_variables, track_roles

Why restrict access true with administer permission ?

Rest looks good to me.

Here's the patch attached that fixed few of the above.

// Naveen

Thanks for your contribution, panshulk!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.