Running coder on this module shows a variety of issues, as well as many @ignore statements used to ignore improper formatting to align text - this isn't a Drupal standard, and should be corrected to bring the module code up to date with current Drupal standards.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aburke626 created an issue. See original summary.

aburke626’s picture

Patch to correct code standards issue and remove unneeded @ignore statements.

A few remaining warnings:

FILE: advagg/advagg_mod/advagg_mod.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 2574 | WARNING | The use of function dprint_r() is discouraged
FILE: advagg/advagg.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 4086 | WARNING | The use of function kprint_r() is discouraged

These seemed to be used properly, as logging functions, but I figured I would put it out there if anyone wanted to re-factor them to get rid of the warnings.

aburke626’s picture

Issue summary: View changes
Status: Active » Needs review
mikeytown2’s picture

No patch has been attached.

In term of kprint_r its being used to generate a nicely formatted error message. I don't see why this is an issue other than coder being extra cautious as you can get a call to undefined function error if you don't do proper checks. I can copy what I have inside of httprl but that would just add more lines of code just to make coder happy.

      if (module_exists('httprl') && function_exists('httprl_pr')) {
        $pretty_data = httprl_pr($data);
      }
      elseif (module_exists('devel') && function_exists('kprint_r')) {
        // @ignore production_php:1
        $pretty_data = kprint_r($data);
      }
      else {
        $pretty_data = '<pre>' . filter_xss(print_r($data, TRUE)) . '</pre>';
      }
      watchdog('advagg_json', 'Error with json encoding the Drupal.settings value. Error Message: %error_message. JSON Data: !data', array(
        '%error_message' => $error_message,
        '!data' => $pretty_data,
      ), WATCHDOG_ERROR);

The dprint_r is used because I mirror the devel module's query log output as that js doesn't work if using defer.

/**
 * Returns the rendered query log.
 */
function advagg_mod_devel_shutdown_query($queries) {
  if (!empty($queries)) {
    if (function_exists('theme_get_registry') && theme_get_registry()) {
      // Safe to call theme('table).
      list($counts) = devel_query_summary($queries);
      $output = devel_query_table($queries, $counts);

      // Save all queries to a file in temp dir. Retrieved via AJAX.
      advagg_mod_devel_query_put_contents($queries);
    }
    else {
      $output = '</div>' . dprint_r($queries, TRUE);
    }
    return $output;
  }
}

I've been using http://pareview.sh/pareview/httpgitdrupalorgprojectadvagggit-7x-2x and it looks like they added in some js checking so I'll work on that as I'm guessing your patch has to do with php :)

aburke626’s picture

Woops, uploaded the patch this time.

mikeytown2’s picture

  1. +++ b/README.txt
    @@ -304,7 +305,19 @@ JAVASCRIPT BOOKMARKLET
    -    javascript:(function(){var loc = document.location.href,qs = document.location.search,regex_off = /\&?advagg=-1/,goto = loc;if(qs.match(regex_off)) {goto = loc.replace(regex_off, '');} else {qs = qs ? qs + '&advagg=-1' : '?advagg=-1';goto = document.location.pathname + qs;}window.location = goto;})();
    +    javascript:(function(){
    +    var loc =
    +    document.location.href,qs =
    +    document.location.search,regex_off =
    +    /\&?advagg=-1/,goto =
    +    loc;
    +    if(qs.match(regex_off)) {
    +      goto = loc.replace(regex_off, '');
    +    } else {
    +      qs = qs ? qs + '&advagg=-1' : '?advagg=-1';
    +      goto = document.location.pathname + qs;
    +    }window.location = goto;
    +    })();
    

    This isn't meant to be read but copy pasted so going over 80 is better.

  2. +++ b/README.txt
    @@ -464,7 +486,8 @@ Issue: https://www.drupal.org/node/2493801
    -https://fourword.fourkitchens.com/article/use-grunt-and-advagg-inline-critical-css-drupal-7-theme
    +https://fourword.fourkitchens.com/article/
    +  use-grunt-and-advagg-inline-critical-css-drupal-7-theme
    

    Splitting up a url is a bad idea. Going over 80 is better for copy/paste.

  3. +++ b/README.txt
    @@ -568,7 +591,8 @@ some tips to hopefully get it working. For Apache enable `mod_rewrite`,
    -    <FilesMatch "^(css|js)__[A-Za-z0-9-_]{43}__[A-Za-z0-9-_]{43}__[A-Za-z0-9-_]{43}.(css|js)(\.gz)?">
    +    <FilesMatch "^(css|js)__[A-Za-z0-9-_]
    +      {43}__[A-Za-z0-9-_]{43}__[A-Za-z0-9-_]{43}.(css|js)(\.gz)?">
    
    @@ -602,7 +626,8 @@ Drupal's core .htaccess file (located at the webroot level).
    -    <FilesMatch "^js__[A-Za-z0-9-_]{43}__[A-Za-z0-9-_]{43}__[A-Za-z0-9-_]{43}.js(\.gz)?">
    +    <FilesMatch "^js__[A-Za-z0-9-_]
    +      {43}__[A-Za-z0-9-_]{43}__[A-Za-z0-9-_]{43}.js(\.gz)?">
    

    This needs to be copy pasted into .htaccess without any modifications. This can not be altered.

  4. +++ b/advagg.admin.js
    @@ -104,20 +104,20 @@ function advagg_toggle_cookie() {
    -    document.cookie = cookie_name + '=;'
    -      + 'expires=Thu, 01 Jan 1970 00:00:00 GMT;'
    -      + ' path=' + Drupal.settings.basePath + ';'
    -      + ' domain=.' + document.location.hostname + ';';
    +    document.cookie = cookie_name + '=;
    +      expires=Thu, 01 Jan 1970 00:00:00 GMT;
    +      path=' + Drupal.settings.basePath + ';
    +      domain=.' + document.location.hostname + ';';
    ...
    -    document.cookie = cookie_name + '=' + Drupal.settings.advagg.key + ';'
    -      + ' expires=' + expire_date.toGMTString() + ';'
    -      + ' path=' + Drupal.settings.basePath + ';'
    -      + ' domain=.' + document.location.hostname + ';';
    +    document.cookie = cookie_name + '=' + Drupal.settings.advagg.key + ';
    +       expires=' + expire_date.toGMTString() + ';
    +       path=' + Drupal.settings.basePath + ';
    +       domain=.' + document.location.hostname + ';';
    

    This code still work?

  5. +++ b/advagg.advagg.inc
    @@ -7,20 +7,18 @@
    -// @ignore sniffer_commenting_functioncomment_hookparamdoc:11
     /**
      * Implements hook_advagg_save_aggregate_alter().
    - *
    - * Used to add in a .gz file if none exits.
    - *
    - * @param array $files_to_save
    - *   Array($uri => $contents).
    - * @param array $aggregate_settings
    - *   Array of settings.
    - * @param array $other_parameters
    - *   Array of containing $files and $type.
      */
     function advagg_advagg_save_aggregate_alter(array &$files_to_save, array $aggregate_settings, array $other_parameters) {
    +  //
    +  // Used to add in a .gz file if none exits.
    +  // @param array $files_to_save
    +  // Array($uri => $contents).
    +  // @param array $aggregate_settings
    +  // Array of settings.
    +  // @param array $other_parameters
    +  // Array of containing $files and $type.
    
    @@ -57,20 +55,17 @@ function advagg_advagg_save_aggregate_alter(array &$files_to_save, array $aggreg
    -// @ignore sniffer_commenting_functioncomment_hookparamdoc:10
     /**
      * Implements hook_advagg_build_aggregate_plans_alter().
    - *
    - * Used to alter the plan so it has the same grouping as cores.
    - *
    - * @param array $files
    - *   List of files in the aggregate as well as the aggregate name.
    - * @param bool $modified
    - *   Change this to TRUE if $files has been changed.
    - * @param string $type
    - *   String containing css or js.
      */
     function advagg_advagg_build_aggregate_plans_alter(array &$files, &$modified, $type) {
    +  // Used to alter the plan so it has the same grouping as cores.
    +  // @param array $files
    +  // List of files in the aggregate as well as the aggregate name.
    +  // @param bool $modified
    +  // Change this to TRUE if $files has been changed.
    +  // @param string $type
    +  // String containing css or js.
    

    This is just trying to make coder happy. For core hooks I agree that duplication isn't needed; for 3rd party hooks it's good to have the documentation in code as https://api.drupal.org/api/drupal/7.x doesn't contain this module. There are other places where this happens; only mentioning it here.

  6. +++ b/advagg.advagg.inc
    @@ -168,18 +163,10 @@ function advagg_advagg_context_alter(&$original, $aggregate_settings, $mode) {
    -      $GLOBALS['conf'][CDN_MODE_VARIABLE] = isset($aggregate_settings['variables'][CDN_MODE_VARIABLE])
    -        ? $aggregate_settings['variables'][CDN_MODE_VARIABLE]
    -        : variable_get(CDN_MODE_VARIABLE, CDN_MODE_BASIC);
    -      $GLOBALS['conf'][CDN_BASIC_FARFUTURE_VARIABLE] = isset($aggregate_settings['variables'][CDN_BASIC_FARFUTURE_VARIABLE])
    -        ? $aggregate_settings['variables'][CDN_BASIC_FARFUTURE_VARIABLE]
    -        : variable_get(CDN_BASIC_FARFUTURE_VARIABLE, CDN_BASIC_FARFUTURE_DEFAULT);
    -      $GLOBALS['conf'][CDN_HTTPS_SUPPORT_VARIABLE] = isset($aggregate_settings['variables'][CDN_HTTPS_SUPPORT_VARIABLE])
    -        ? $aggregate_settings['variables'][CDN_HTTPS_SUPPORT_VARIABLE]
    -        : variable_get(CDN_HTTPS_SUPPORT_VARIABLE, FALSE);
    -      $GLOBALS['conf'][CDN_STATUS_VARIABLE] = isset($aggregate_settings['variables'][CDN_STATUS_VARIABLE])
    -        ? $aggregate_settings['variables'][CDN_STATUS_VARIABLE]
    -        : variable_get(CDN_STATUS_VARIABLE, CDN_DISABLED);
    +      $GLOBALS['conf'][CDN_MODE_VARIABLE] = isset($aggregate_settings['variables'][CDN_MODE_VARIABLE]) ? $aggregate_settings['variables'][CDN_MODE_VARIABLE] : variable_get(CDN_MODE_VARIABLE, CDN_MODE_BASIC);
    +      $GLOBALS['conf'][CDN_BASIC_FARFUTURE_VARIABLE] = isset($aggregate_settings['variables'][CDN_BASIC_FARFUTURE_VARIABLE]) ? $aggregate_settings['variables'][CDN_BASIC_FARFUTURE_VARIABLE] : variable_get(CDN_BASIC_FARFUTURE_VARIABLE, CDN_BASIC_FARFUTURE_DEFAULT);
    +      $GLOBALS['conf'][CDN_HTTPS_SUPPORT_VARIABLE] = isset($aggregate_settings['variables'][CDN_HTTPS_SUPPORT_VARIABLE]) ? $aggregate_settings['variables'][CDN_HTTPS_SUPPORT_VARIABLE] : variable_get(CDN_HTTPS_SUPPORT_VARIABLE, FALSE);
    +      $GLOBALS['conf'][CDN_STATUS_VARIABLE] = isset($aggregate_settings['variables'][CDN_STATUS_VARIABLE]) ? $aggregate_settings['variables'][CDN_STATUS_VARIABLE] : variable_get(CDN_STATUS_VARIABLE, CDN_DISABLED);
    

    I think this is less readable; now you have to scroll.

  1. +++ b/advagg.inc
    @@ -472,8 +472,7 @@ function &advagg_load_files_info_into_static_cache(array $files) {
    -    // @ignore sniffer_whitespace_openbracketspacing_openingwhitespace
    -    if ( !empty($cached_data)
    +    if (!empty($cached_data)
           && !empty($cached_data[$cache_id])
         ) {
    
    @@ -550,8 +549,7 @@ function advagg_get_info_on_files(array $files, $bypass_cache = FALSE, $run_alte
    -    // @ignore sniffer_whitespace_openbracketspacing_openingwhitespace
    -    if ( $bypass_cache == FALSE
    +    if ($bypass_cache == FALSE
           && is_array($cached_data)
           && array_key_exists($cache_id, $cached_data)
         ) {
    
    @@ -634,8 +632,7 @@ function advagg_get_info_on_files(array $files, $bypass_cache = FALSE, $run_alte
    -      // @ignore sniffer_whitespace_openbracketspacing_openingwhitespace
    -      if ( empty($info['#no_cache'])
    +      if (empty($info['#no_cache'])
             && !empty($info['cache_id'])
             && (empty($cached_data[$info['cache_id']]) || $info !== $cached_data[$info['cache_id']])
           ) {
    
    @@ -756,7 +753,7 @@ function advagg_generate_groups(array $files_to_aggregate, $type) {
    -  if ( variable_get('advagg_ie_css_selector_limiter', ADVAGG_IE_CSS_SELECTOR_LIMITER)
    +  if (variable_get('advagg_ie_css_selector_limiter', ADVAGG_IE_CSS_SELECTOR_LIMITER)
         || variable_get('advagg_browser_dns_prefetch', ADVAGG_BROWSER_DNS_PREFETCH)
       ) {
    
    @@ -912,7 +909,7 @@ function advagg_generate_groups(array $files_to_aggregate, $type) {
    -        if ( variable_get('advagg_browser_dns_prefetch', ADVAGG_BROWSER_DNS_PREFETCH)
    +        if (variable_get('advagg_browser_dns_prefetch', ADVAGG_BROWSER_DNS_PREFETCH)
               && isset($files_info[$file_info['data']]['dns_prefetch'])
             ) {
    
    @@ -1366,10 +1363,9 @@ function advagg_create_aggregate_files(array $plans, $type) {
    -  // @ignore sniffer_whitespace_openbracketspacing_openingwhitespace
    -  if ( module_exists('httprl')
    +  if (module_exists('httprl')
         && variable_get('advagg_use_httprl', ADVAGG_USE_HTTPRL)
    -    && ( is_callable('httprl_is_background_callback_capable')
    +    && (is_callable('httprl_is_background_callback_capable')
           && httprl_is_background_callback_capable()
           || !is_callable('httprl_is_background_callback_capable')
           )
    
    +++ b/advagg.install
    @@ -707,7 +707,7 @@ function advagg_install_change_table_collation($table_name, array $fields, $coll
    -    if ( isset($schema_fields[$row->Field])
    +    if (isset($schema_fields[$row->Field])
           && is_array($schema_fields[$row->Field])
           && array_key_exists('default', $schema_fields[$row->Field])
         ) {
    
    @@ -982,7 +982,7 @@ function advagg_requirements($phase) {
    -  if ( empty($item_css['page_callback'])
    +  if (empty($item_css['page_callback'])
         || strpos($item_css['page_callback'], 'advagg') === FALSE
         || empty($item_js['page_callback'])
         || strpos($item_js['page_callback'], 'advagg') === FALSE
    
    @@ -1000,8 +1000,7 @@ function advagg_requirements($phase) {
    -  // @ignore sniffer_whitespace_openbracketspacing_openingwhitespace
    -  if ( empty($styles_info['#pre_render'])
    +  if (empty($styles_info['#pre_render'])
         || !is_array($styles_info['#pre_render'])
         || !in_array('advagg_modify_css_pre_render', $styles_info['#pre_render'])
         || empty($scripts_info['#pre_render'])
    
    @@ -1042,10 +1041,9 @@ function advagg_requirements($phase) {
    -  // @ignore sniffer_whitespace_openbracketspacing_openingwhitespace
       $search404_ignore_query = variable_get('search404_ignore_query', 'gif jpg jpeg bmp png');
    -  if ( module_exists('search404') &&
    -    (  strpos($search404_ignore_query, 'css') === FALSE
    +  if (module_exists('search404') &&
    +    (strpos($search404_ignore_query, 'css') === FALSE
         || strpos($search404_ignore_query, 'js') === FALSE
         )
       ) {
    
    @@ -1070,9 +1068,8 @@ function advagg_requirements($phase) {
    -  // @ignore sniffer_whitespace_openbracketspacing_openingwhitespace:2
    -  if ( empty($GLOBALS['is_https']) &&
    -    (  (isset($_SERVER['HTTPS']) &&  strtolower($_SERVER['HTTPS']) === 'on')
    +  if (empty($GLOBALS['is_https']) &&
    +    ((isset($_SERVER['HTTPS']) &&  strtolower($_SERVER['HTTPS']) === 'on')
         || (isset($_SERVER['HTTP_X_FORWARDED_PROTO']) && $_SERVER['HTTP_X_FORWARDED_PROTO'] === 'https')
         || (isset($_SERVER['HTTP_HTTPS']) && $_SERVER['HTTP_HTTPS'] === 'on')
         )
    
    @@ -1082,7 +1079,7 @@ function advagg_requirements($phase) {
    -        '@code' => 'if ( (isset($_SERVER[\'HTTPS\']) && strtolower($_SERVER[\'HTTPS\']) == \'on\')
    +        '@code' => 'if ((isset($_SERVER[\'HTTPS\']) && strtolower($_SERVER[\'HTTPS\']) == \'on\')
       || (isset($_SERVER[\'HTTP_X_FORWARDED_PROTO\']) && $_SERVER[\'HTTP_X_FORWARDED_PROTO\'] == \'https\')
       || (isset($_SERVER[\'HTTP_HTTPS\']) && $_SERVER[\'HTTP_HTTPS\'] == \'on\')
     ) {
    

    Multi-line if statements; I think the way it's currently written is easier to read, but this change doesn't affect readability that much so I think I'd be ok with all of these changes (the bulk of this patch).

  2. +++ b/advagg.install
    @@ -1259,13 +1256,13 @@ function advagg_install_check_via_http(array &$requirements) {
    -      //  Not a 404 OR
    -      //  No data returned OR
    -      //   Headers do not contain "x-advagg" AND
    -      //   Body does not contain "advagg_missing_fast404".
    -      if ( $request->code != 404
    +      // Not a 404 OR
    +      // No data returned OR
    +      // Headers do not contain "x-advagg" AND
    +      // Body does not contain "advagg_missing_fast404".
    +      if ($request->code != 404
             || empty($request->data)
    -        || ( empty($request->headers['x-advagg'])
    +        || (empty($request->headers['x-advagg'])
               && strpos($request->data, '<!-- advagg_missing_fast404 -->') === FALSE
               )
           ) {
    

    I think having the comments match the if statement hierarchy makes it easier to read. Not willing to do this change.

  3. +++ b/advagg.module
    @@ -650,16 +641,13 @@ function advagg_menu() {
    -// @ignore sniffer_commenting_functioncomment_hookparamdoc:7
     /**
      * Implements hook_cron().
    - *
    - * This will be ran once a day at most.
    - *
    - * @param bool $bypass_time_check
    - *   Set to TRUE to skip the 24 hour check.
      */
     function advagg_cron($bypass_time_check = FALSE) {
    +  // This will be run once a day at most.
    +  // @param bool $bypass_time_check
    +  // Set to TRUE to skip the 24 hour check.
    
    @@ -695,16 +683,14 @@ function advagg_cron($bypass_time_check = FALSE) {
    -// @ignore sniffer_commenting_functioncomment_hookparamdoc:5
     /**
      * Implements hook_flush_caches().
    - *
    - * @param bool $all_bins
    - *   TRUE: Get all advagg cache bins.
    - * @param bool $push_new_changes
    - *   FALSE: Do not scan for changes.
      */
     function advagg_flush_caches($all_bins = FALSE, $push_new_changes = TRUE) {
    +  // @param bool $all_bins
    +  // TRUE: Get all advagg cache bins.
    +  // @param bool $push_new_changes
    +  // FALSE: Do not scan for changes.
    

    Extending core hooks to do more than what they originally did; documenting this in doxygen format is the right thing to do. Not willing to do this change.

  4. +++ b/advagg_bundler/advagg_bundler.module
    @@ -197,6 +197,47 @@ function advagg_bundler_analysis($filename = '', $force = FALSE, $safesql = FALS
    + * SELECT
    + *  af.filename AS filename,
    + *  af.filesize AS filesize,
    + *  af.mtime AS mtime,
    + *  af.changes AS changes,
    + *  af.linecount AS linecount,
    + *  af.filename_hash AS filename_hash,
    + *  aa.counter AS counter,
    + *  aa.hashlist AS hashlist
    + * FROM advagg_files af
    + * INNER JOIN (
    + * SELECT
    + *  aa.filename_hash AS filename_hash,
    + *  LPAD(CAST(COUNT(aav.aggregate_filenames_hash) AS char(8)), 8, '0') AS
    + *  counter,
    + *  HEX(SHA1(GROUP_CONCAT(DISTINCT aa.aggregate_filenames_hash
    + * ORDER BY aa.aggregate_filenames_hash ASC))) AS hashlist
    + * FROM advagg_aggregates aa
    + * INNER JOIN advagg_aggregates_versions aav
    + * ON aav.aggregate_filenames_hash = aa.aggregate_filenames_hash
    + * AND aav.root = 1
    + * AND aav.atime > (UNIX_TIMESTAMP() - 1209600)
    + * GROUP BY aa.filename_hash
    + *  aa ON af.filename_hash = aa.filename_hash
    

    Wrap this in @code @endcode. Good idea!

  5. +++ b/advagg_font/advagg_font.install
    @@ -43,12 +43,14 @@ function advagg_font_requirements($phase) {
    -        $t('Directions on how to install it can be found on the <a href="@url">Async Font Loader</a> settings page', array('@url' => url('admin/config/development/performance/advagg/font'))) :
    -        $t('Please go to the <a href="@url">Async Font Loader</a> settings page and choose inline or local.', array('@url' => url('admin/config/development/performance/advagg/font'))),
    +      $t('Directions on how to install it can be found on the <a href="@url">Async Font Loader</a> settings page', array(
    +        '@url' => url('admin/config/development/performance/advagg/font'))) :
    +      $t('Please go to the <a href="@url">Async Font Loader</a> settings page and choose inline or local.', array(
    +        '@url' => url('admin/config/development/performance/advagg/font'))),
    

    The array should be fully multi-line with a comma after url().

  6. +++ b/advagg_mod/advagg_mod.admin.inc
    @@ -647,8 +646,7 @@ function advagg_mod_admin_test_css_files() {
    -      // @ignore sniffer_semantics_functioncall_notliteralstring
    -      $after = t($before);
    +      $after = t('@before', array('@before' => $before));
    
    +++ b/advagg_mod/advagg_mod.advagg.inc
    @@ -82,8 +81,7 @@ function advagg_mod_advagg_css_content_t_replace_callback(array $matches) {
    -  // @ignore sniffer_semantics_functioncall_notliteralstring
    -  $after = t($before);
    +  $after = t('@before', array('@before' => $before));
    

    This change can't be done. The text needs to be ran through t().

  7. +++ b/advagg_mod/advagg_mod.module
    @@ -2443,21 +2429,18 @@ function advagg_mod_js_inline_processor(&$html) {
    - *
    - * @param array $magic_settings
    - *   The renderable form array of the magic module theme settings. READ ONLY.
    - * @param string $theme
    - *   The theme that the settings will be editing.
    - *
    - * @return array
    - *   The array of settings within the magic module theme page. Must not contain
    - *   anything from the $magic_settings array.
      */
     function advagg_mod_magic(array $magic_settings, $theme) {
    +  // @param array $magic_settings
    +  // The renderable form array of the magic module theme settings. READ ONLY.
    +  // @param string $theme
    +  // The theme that the settings will be editing.
    +  //
    +  // @return array
    +  // The array of settings within the magic module theme page. Must not contain
    +  // anything from the $magic_settings array.
    

    This can just be removed as magic has an api file http://cgit.drupalcode.org/magic/tree/magic.api.php?h=7.x-1.x

    The note about the $magic_settings being ready only and how it can't be in the return array should be kept though. Put the read only at the top and the part about the return at the bottom above return.

mikeytown2’s picture

js changes. coder will still complain about the minified js though.

  • mikeytown2 committed 5ae06e7 on 7.x-2.x
    Issue #2744395 by mikeytown2: Code Standards Cleanup
    
mikeytown2’s picture

Status: Needs review » Needs work

#7 has been committed; setting issue to needs work from the comments in #6.

  • mikeytown2 committed 2200664 on 7.x-2.x
    Issue #2744395 by mikeytown2: use single quotes for use strict; detect...
mikeytown2’s picture

mikeytown2’s picture

@aburke626
Any update on the issues I brought up in #6?

mikeytown2’s picture

@aburke626
Any update on the issues I brought up in #6?

Should I unassign you from this issue?

mikeytown2’s picture

Assigned: aburke626 » Unassigned

unassigning aburke626 due to lack of feedback. Will work on the changes suggested and issue credit where due.

mikeytown2’s picture

Re-roll of #5 with the comments from #6 taken into account.

mikeytown2’s picture

mikeytown2’s picture

Status: Needs work » Needs review

The last submitted patch, 11: advagg-2744395-10-use-strict-js.patch, failed testing.

The last submitted patch, 11: advagg-2744395-10-use-strict-js.patch, failed testing.

The last submitted patch, 11: advagg-2744395-10-use-strict-js.patch, failed testing.

mikeytown2’s picture

Status: Needs review » Active

#16 has been committed. Thanks for bringing this up!

mikeytown2’s picture

Status: Active » Fixed

Status: Fixed » Closed (fixed)

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