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

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.

Thanks for pointing me to the automated review tool.

I have fixed all errors reported by the tool.

@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.

Issue summary:View changes

Fixed branch names.

Issue summary:View changes
Status:Needs work» Needs review

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

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

Both 1 and 2 are fixed.

Yottaa

Status:Needs work» Needs review

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.

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

Status:Needs review» Reviewed & tested by the community

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

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.

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.