[3.0.x] CloudFront Cache Path Invalidate

Hi Prashant Mishra,

Kindly fix some coding standards issues.

FILE: .../web/modules/contrib/cloudfront_cache_path_invalidate/README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 15 | WARNING | Line exceeds 80 characters; contains 116 characters
----------------------------------------------------------------------


FILE: ...cache_path_invalidate/src/Form/CloudfrontCacheInvalidateForm.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 44 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 44 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...ache_path_invalidate/src/Form/AutoCloudfrontCacheSettingForm.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
----------------------------------------------------------------------
 34 | ERROR | [ ] Doc comment short description must start with a
    |       |     capital letter
 34 | ERROR | [x] Doc comment short description must end with a full
    |       |     stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 385ms; Memory: 10Mb

Hi Prashant,

Thanks for your contribution !

Fix below issue : cloudfront_cache_path_invalidate.module line no. 266

Call to deprecated function db_or(): in drupal:8.0.0 and is removed from drupal:9.0.0. Create a \Drupal\Core\Database\Query\Condition object, specifying an OR conjunction: new Condition('OR');

Thank you for applying! I added the Git instructions for non-maintainer users and the PAReview checklist link.

If you haven't done it, yet, please check the PAReview report and fix what needs to be fixed. There could be some false positives; verify that what reported is correct, before making any change.

What reported about the comment not starting with a capital letter is caused from the comment containing {@inheritDoc} instead of {@inheritdoc}. The warning message is not clear, probably because PAReview first checks the comment contains {@inheritdoc} (in a case-sensitive way) and then checks if the first character in the comment is a capital letter. ({ isn't a capital letter.)

  /**
   * {@inheritDoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('messenger'),
      $container->get('logger.factory')->get('cloudfront_cache_path_invalidate')
    );
  }

The same is true for the other warning message (comment short description must end with a full stop).

In short, change the comment to the following, and the warnings will go away.

  /**
   * {@inheritdoc}
   */

The task of reviewers is pointing out what needs to be fixed, not providing patches to fix what they think should be changed, or what other users reported that should be changed.

@Prashant Mishra If you fixed everything reviewers reported, please change the status to Needs review or reviewers will not know the project is ready to be reviewed again.

Hi,
It seems all issues has been fixed.

I ordered the reports basing on the importance of the issue. They aren't ordered basing on the file containing the code.

    // Logs a message.
    if ($total_paths == 1) {
      \Drupal::logger('cloudfront_cache_path_invalidate')->notice('The URL ' . $paths[0] . ' is in progress.');
    }
    else {
      foreach ($paths as $value) {
        \Drupal::logger('cloudfront_cache_path_invalidate')->notice('The URL ' . $value . ' is in progress.');
      }
    }

Instead of concatenating strings, the code should use placeholders, which allow to sanitize the dynamic part of the string.
The URL is in progress. doesn't make sense; it's the operation on that URL that is in progress. The log purpose is probably showing which URLs are handled, not informing they are being processed; in that case, I would rather use The URL %url is invalidated.

    catch (\Exception $e) {
      $this->logger->error('Message from @module: @message.', [
        '@module' => 'cloudfront_cache_path_invalidate',
        '@message' => $e->getMessage(),
      ]);
      $this->messenger->addMessage($e->getMessage(), $this->messenger::TYPE_ERROR);
    }

The string passed to $this->logger->error() is correct, as it uses placeholders which allow to sanitize the exception message to insert it into HTML markup . The module name isn't necessary, as the log messages go to a specific channel used from the module (cloudfront_cache_path_invalidate). I would also use a less generic phrase, for example An error occurred while […]: @message.
The same string used for $this->logger->error() should be passed as first argument to $this->messenger->addMessage().

  if (!in_array('anonymous', $current_user->getRoles())) {

To check if $current_user</em> is an object for the anonymous user, it's enough to call <a href="https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Session%21AccountInterface.php/function/AccountInterface%3A%3AisAnonymous/8.9.x"><code>$current_user->isAnonymous().

      else {
        $form_state->setErrorByName('ec_fieldset[' . $key . '][ec_cloudfront_url]', $this->t('The Cloudfront URL introduced is not valid.'));
      }

introduced isn't necessary: 'The Cloudfront URL isn't valid.

  catch (AwsException $e) {
    $catch = TRUE;
    $return[1] = $e->getMessage();

    // Logs an error.
    \Drupal::logger('cloudfront_cache_path_invalidate')->error($e->getMessage());
  }

  if (empty($catch)) {
    $return[0] = TRUE;

    // Logs a message.
    if ($total_paths == 1) {
      \Drupal::logger('cloudfront_cache_path_invalidate')->notice('The URL ' . $paths[0] . ' is in progress.');
    }
    else {
      foreach ($paths as $value) {
        \Drupal::logger('cloudfront_cache_path_invalidate')->notice('The URL ' . $value . ' is in progress.');
      }
    }
  }

catch()</em> sections can return; it's useless to set <code>$catch and then check that variable isn't initialized.

  catch (AwsException $e) {
    $return[1] = $e->getMessage();

    \Drupal::logger('cloudfront_cache_path_invalidate')->error($e->getMessage());
    return $return;
  }

  $return[0] = TRUE;

  if ($total_paths == 1) {
    \Drupal::logger('cloudfront_cache_path_invalidate')->notice('The URL ' . $paths[0] . ' is in progress.');
  }
  else {
    foreach ($paths as $value) {
      \Drupal::logger('cloudfront_cache_path_invalidate')->notice('The URL ' . $value . ' is in progress.');
    }
  }

  return $return;

I removed the comments since it's clear what the next line is doing.

(It's also useless to have code that specifically handles the case of an array with a single item. foreach ($paths as $value) { } works also for arrays with a single item or empty arrays.)

The string passed to error() isn't using placeholders.

/**
 * Implements hook_entity_update().
 */
function cloudfront_cache_path_invalidate_entity_insert(EntityInterface $entity) {

The document code is wrong: The hook is hook_entity_insert().

      $detail_page ? array_push($paths, $detail_page) : $paths;

While in some cases the ternary operator can replace an if() statement, that is a case where it doesn't make sense. It's better to use an if() statement.

if ($detail_page) {
  array_push($paths, $detail_page);
}

/**
 * Hook Entity action cache clear common code.
 *
 * @param string $entity_type
 *   Entity Type.
 * @param string $bundle_type
 *   Bundle Type.
 * @param string $bundle
 *   Bundle Name.
 * @param string $entity_id
 *   Entity Id.
 * @param Drupal\Core\Entity\EntityInterface $entity
 *   Entity.
 *
 * @return array
 *   Cloudfront invalidate paths array.
 */
function cloudfront_cache_path_invalidate_entity_actions($entity_type, $bundle_type, $bundle, $entity_id, EntityInterface $entity) {

  if (empty($entity_type) && $bundle_type == 'menu_link_content') {
    $entity_type = 'menu';
    $bundle_type = $entity->getMenuName();
  }

  return cloudfront_cache_path_invalidate_settings($entity_type, $bundle_type, $bundle, $entity_id, $entity);

}

If that is a hook implementation, the document comment should simply be Implements hook_entity_actions(). In this case, it should not be called as a normal function as the module does. I take that isn't really a hook implementation, and the short documentation line should be different from Hook Entity action cache clear common code.

CONFIGURATION
================================================================================
This needs the following settings:

Cloudfront/AWS settings(Sample Credential)
$settings['aws.distributionid'] = 'UQWUQW55UQUQW';
$settings['aws.region'] = 'ap-south-1';
$settings['s3fs.access_key'] = 'KITCFZKITCF423AKIAIZ';
$settings['s3fs.secret_key'] = 'tv8T5IaW3rttv8T5Irtv8T5I/tv8T5Itv8T5I';

The README.txt file should explain why those settings are necessary and explain if those are eventually test values the users can use to test the module. (I find hard to believe that all the users using this module should need to use the same secret key which, at this point, isn't a secret key anymore.)

/**
 * Implements hook_help().
 */
function cloudfront_cache_path_invalidate_help($route_name, RouteMatchInterface $route_match) {
  switch ($route_name) {
    case 'help.page.cloudfront_cache_path_invalidate':

      $filepath = dirname(__FILE__) . '/README.md';
      if (file_exists($filepath)) {
        $readme = file_get_contents($filepath);
      }
      else {
        $filepath = dirname(__FILE__) . '/README.txt';
        if (file_exists($filepath)) {
          $readme = file_get_contents($filepath);
        }
      }
      if (!isset($readme)) {
        return NULL;
      }
      if (\Drupal::moduleHandler()->moduleExists('markdown')) {
        $filters = module_invoke('markdown', 'filter_info');
        $info = $filters['filter_markdown'];

        if (function_exists($info['process callback'])) {
          $output = $info['process callback']($readme, NULL);
        }
        else {
          $output = '<pre>' . $readme . '</pre>';
        }
      }
      else {
        $output = '<pre>' . $readme . '</pre>';
      }

      return $output;
  }
}

I have seen many similar implementations. Showing the README.txt or README.md file when the module is already installed doesn't make much sense, as that file is usually supposed to give information necessary before installing the module (for example, the dependencies necessary to run the module), which isn't anymore necessary when the module is installed.
If then the users don't have the Markdown module installed, what they see in the page is rather "ugly."

cloudfront_cache_path_invalidate.admin_cloudfront_invalidate_url:
  path: '/admin/config/services/cloudfront-invalidate-url'
  defaults:
    _form: '\Drupal\cloudfront_cache_path_invalidate\Form\CloudfrontCacheInvalidateForm'
    _title: 'Cloudfront Cache Setting'
  requirements:
    _role: 'administrator'
cloudfront_cache_path_invalidate.auto_admin_cloudfront_url_invalidate_setting:
  path: '/admin/config/services/auto-cloudfront-cache-entities'
  defaults:
    _form: '\Drupal\cloudfront_cache_path_invalidate\Form\AutoCloudfrontCacheSettingForm'
    _title: 'Automatic Cloudfront Cache Setting'
  requirements:
    _role: 'administrator'

While it's possible to define which roles the users accessing a route needs to have to, it's rather preferable to use a permission the users need to have, especially when the role is created from the installation profile. If users are using a different installation profile which doesn't create that role, or future Drupal versions remove that role, the module would not be usable.

Until the role isn't given, the application isn't fixed.

No, you don't have the vetted role.

If you aren't interested anymore in getting the vetted role and be able to opt into security coverage for the projects you create/created, that's fine. In that case, the status is Closed (won't fix), not Fixed.

Please ignore this patch.

Patch for module upgrade to make it compatible with drupal 9 & 10.

The user who created this application closed it, which means he was not anymore interested in proceeding with it.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the Slack #contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the reviewers.