Yottaa eComet is a new Drupal plugin that integrates Yottaa Site Optimizer into your Drupal admin page. Yottaa speeds up page load times, creates higher conversion rates and increases user engagement. This plugin makes it even easier for Drupal users to improve their site performance with Yottaa.

Whether you're already a Yottaa Site Optimizer user or want to try it for the first time, you'll enjoy the ease of having a Yottaa control panel right on your Drupal admin panel.

Project Home page:
http://documentation.yottaa.com/display/DOC/Yottaa+eComet+for+Drupal+and+Drupal+Commerce

Project Git link:

git clone --branch 7.x-1.x git.drupal.org:sandbox/Yottaa/2076983.git yottaa_optimizer

git clone --branch 6.x-1.x git.drupal.org:sandbox/Yottaa/2076983.git yottaa_optimizer

Comments

PA robot’s picture

Status:Needs review» Needs work

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

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.

Yottaa’s picture

Thanks for pointing me to the automated review tool.

I have fixed all errors reported by the tool.

asherry’s picture

@Yottaa, props for tackling a d6 and d7 version in the same application. That's tough.

1 - I'm a little confused by the directory structure of your module. Are the .md, docs, and dist folders all needed to enable this module? It looks like all the git source files have been included.
2 - You can take out the 'Yottaa@' in your project links. That will force a user to clone as your username, and ask for a password.
3 - After you're done fixing something in this queue, you need to set the status to "Needs Review". If you don't, nobody will look at your module. "Needs work" implies that you're still working on something, or a reviewer has suggested you to fix something else.

asherry’s picture

Issue summary:View changes

Fixed branch names.

Yottaa’s picture

Issue summary:View changes
Status:Needs work» Needs review
alexverb’s picture

Status:Needs review» Needs work

Hi Yottaa,

I found the following issues,

  • I share the same opinion as asherry regarding the modules directory structure. Couldn't you just have called the module yottaa or yottaa_optimizer and remove the unneeded modules, dist and docs folder? I don't know of any other modules that have the main.module file not in the root directory.
  • Also when using the t() function in combination with variables you should always use placeholders that automatically sanitize it's text for you. Read more about it here: https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7
  • And at line 550 you have a variable_set function that does not prefix the variable name with the module name, namely the 'reverse_proxy' variable. That could conflict eventually with other modules. In this case it's highly unlikely unless D7 core is using a reverse proxy. But it's just best practice to avoid name collisions.

For the rest your code looks pretty clean. I'll try to test some functionality within the next few days.

Greetz, Alex

Yottaa’s picture

Hi, Alex

Thank you very much for helping us review the module.

For the third issue, we do need to set the global variable since Yottaa is basically a reverse proxy. Please let us know if you want to learn more about Yottaa Optimizer services.

Yottaa

Yottaa’s picture

Both 1 and 2 are fixed.

Yottaa

Yottaa’s picture

Status:Needs work» Needs review
pachabhaiya’s picture

Hi Yottaa,
pareview.sh still shows some error in a file 'yottaa_optimizer.module'.

569 | WARNING | A comma should follow the last multiline array item. Found:
| | 'textarea'
530 | WARNING | All variables defined by your module must be prefixed with
| | your module's name to avoid name collisions with others.
| | Expected start with "yottaa_optimizer" but found
| | "reverse_proxy"

Since you are using the multi-line array, each and every line in the array should be followed by a comma. Adding comma at the end of line 569, will help you get rid of that error message.

$form['yottaa_optimizer_purge_cache_paths'] = array(
    '#type' => 'textarea',
);

For the error in line 530, it is always a good practice for all the variables defined by your module to be prefixed with your module's name. If any other module is using the same variable name and is already installed, then you might face problem of name collision with your module.

Yottaa’s picture

Hi, Thanks for the review.

We have fixed warnings for line 569.

As for 530, we do need to overwrite the global setting in order to get the Yottaa services working.

If you would like to learn more about our technologies and services, please let us know.

Thanks!

Yottaa

Yottaa’s picture

Status:Needs review» Reviewed & tested by the community
klausi’s picture

Status:Reviewed & tested by the community» Needs review

Please don't RTBC your own issue, see https://drupal.org/node/532400

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

mike.roberts’s picture

Upon reviewing the issue with with Line 530 (The reverse_proxy variable debacle) I'm pretty sure there is no reason why you can't name the variable yottaa_optimizer_reverse_proxy.

README.md
Leave this as is.

<?php
$conf
['reverse_proxy'] = TRUE;
$conf['reverse_proxy_addresses'] = isset($_SERVER['REMOTE_ADDR']) ? array($_SERVER['REMOTE_ADDR']) : array();
?>

yottaa_optimizer.module
change this:

<?php
'#default_value' => variable_get('reverse_proxy', 0),
?>

to this:

<?php
'#default_value' => variable_get('yottaa_optimizer_reverse_proxy', 0),
?>

change this:

<?php
variable_set
('reverse_proxy', intval($form_state['values']['yottaa_optimizer_enable_proxy_mode']));
?>

to this:

<?php
variable_set
('yottaa_optimizer_reverse_proxy', intval($form_state['values']['yottaa_optimizer_enable_proxy_mode']));
?>

yottaa_optimizer.theme.inc
change this:

<?php
if (variable_get('reverse_proxy', 0) == 0 && $yottaa_optimizer_api->isLive($yottaa_optimizer_status)) {
?>

to this:

<?php
if (variable_get('yottaa_optimizer_reverse_proxy', 0) == 0 && $yottaa_optimizer_api->isLive($yottaa_optimizer_status)) {
?>

After those changes everything should stay the same and works as before. Now you're following coding standards and making the people who are in charge of getting your project into a release happier.

You should also think about fixing your folder structure like everyone else above me has mentioned.

Edits: Correcting myself and making changes more clear.

mike.roberts’s picture

I also found this to be a little weird:

<?php
 
if (!isset($json_output["error"])) {
   
$yottaa_optimizer_preview_url = $json_output["preview_url"];
    if (
$yottaa_optimizer_status == 'preview') {
     
$output['status'] = array(
       
'#type' => 'markup',
       
'#markup' => '<div>Your site is currently in <span class="status-preview">' . $yottaa_optimizer_status . '</span>. This allows you to access an optimized'
       
. ' version of your website using the preview URL (<a href="' . $yottaa_optimizer_preview_url . '" target="_blank">' . $yottaa_optimizer_preview_url . '</a>).'
       
. ' Before making your site live look over the links and configuration below.</div>',
      );
    }
    elseif (
$yottaa_optimizer_api->isLive($yottaa_optimizer_status)) {
     
$output['status'] = array(
       
'#type' => 'markup',
       
'#markup' => '<div>Your site is currently in <span class="status-live">Live</span>.</div>',
      );
    }
    elseif (
$yottaa_optimizer_api->isBypass($yottaa_optimizer_status)) {
     
$output['status'] = array(
       
'#type' => 'markup',
       
'#markup' => '<div>Your site is currently in <span class="status-paused">Bypass Mode</span>.</div>',
      );
    }
    elseif (
$yottaa_optimizer_api->isTransparent($yottaa_optimizer_status)) {
     
$output['status'] = array(
       
'#type' => 'markup',
       
'#markup' => '<div>Your site is currently in <span class="status-paused">Transparent Proxy Mode</span>.</div>',
      );
    }
  }
?>
  • You're not using the isPreview method for the first check, is there a reason for that?
  • You're printing out $yottaa_optimizer_status in #markup if it's in preview, but using harcoded "Live, Bypass, or Transparent Proxy Mode" for the later ones. You should make this code section more consistent.
heddn’s picture

Status:Needs review» Needs work
PAReview: Individual user account
It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.
PA robot’s picture

Status:Needs work» Closed (won't fix)

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.