[1.0.x] Xray Audit

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

To reviewers: Please read How to review security advisory coverage applications → , What to cover in an application review → , and Drupal.org security advisory coverage application workflow → .

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

It seems you have missed working on the coding standards. You can use the PHPCS tool for checking and resolving issues.

phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml xray_audit/

FILE: xray_audit/assets/css/xray-audit.css
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 2 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: xray_audit/README.md
----------------------------------------------------------------------
FOUND 0 ERRORS AND 19 WARNINGS AFFECTING 19 LINES
----------------------------------------------------------------------
   5 | WARNING | Line exceeds 80 characters; contains 126 characters
  12 | WARNING | Line exceeds 80 characters; contains 112 characters
  15 | WARNING | Line exceeds 80 characters; contains 117 characters
  25 | WARNING | Line exceeds 80 characters; contains 85 characters
  27 | WARNING | Line exceeds 80 characters; contains 109 characters
  30 | WARNING | Line exceeds 80 characters; contains 102 characters
  46 | WARNING | Line exceeds 80 characters; contains 123 characters
  49 | WARNING | Line exceeds 80 characters; contains 122 characters
  63 | WARNING | Line exceeds 80 characters; contains 123 characters
  66 | WARNING | Line exceeds 80 characters; contains 116 characters
  68 | WARNING | Line exceeds 80 characters; contains 82 characters
  72 | WARNING | Line exceeds 80 characters; contains 123 characters
  73 | WARNING | Line exceeds 80 characters; contains 129 characters
  76 | WARNING | Line exceeds 80 characters; contains 131 characters
  85 | WARNING | Line exceeds 80 characters; contains 96 characters
 106 | WARNING | Line exceeds 80 characters; contains 127 characters
 107 | WARNING | Line exceeds 80 characters; contains 102 characters
 123 | WARNING | Line exceeds 80 characters; contains 175 characters
 129 | WARNING | Line exceeds 80 characters; contains 81 characters
----------------------------------------------------------------------


FILE: xray_audit/src/Plugin/tasks/QueryGroup/XrayAuditQueryTaskMediaPlugin.php
--------------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------
 44 | ERROR | [x] Expected 1 space before "="; 0 found
 45 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "NULL" but found "null"
--------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------


FILE: xray_audit/src/Plugin/tasks/QueryGroup/XrayAuditQueryTaskNodePlugin.php
-------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
-------------------------------------------------------------------------------------------------------------
  18 | ERROR | [x] Doc comment star missing
  76 | ERROR | [x] Expected 1 space after FOREACH keyword; 0 found
  82 | ERROR | [x] Expected 1 space after FOREACH keyword; 0 found
 144 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "NULL" but found "null"
-------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------


FILE: xray_audit/src/Plugin/tasks/QueryGroup/XrayAuditQueryTaskParagraphsPlugin.php
-------------------------------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
-------------------------------------------------------------------------------------------------------------------
 50 | ERROR | [x] Expected 1 space after "="; 2 found
 51 | ERROR | [x] Expected 1 space after "="; 2 found
 85 | ERROR | [x] Expected 1 space after "="; 2 found
 86 | ERROR | [x] Expected 1 space after "="; 2 found
 96 | ERROR | [x] Expected 1 space after "="; 0 found
 97 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "NULL" but found "null"
-------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------------------


FILE: xray_audit/xray_audit.info.yml
------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 4 WARNINGS AFFECTING 3 LINES
------------------------------------------------------------------------------------------------------------------
  1 | WARNING | [ ] Remove "project" from the info file, it will be added by drupal.org packaging automatically
  1 | WARNING | [ ] Remove "datestamp" from the info file, it will be added by drupal.org packaging automatically
  1 | WARNING | [ ] Remove "version" from the info file, it will be added by drupal.org packaging automatically
  7 | WARNING | [ ] All dependencies must be prefixed with the project name, for example "drupal:"
 11 | ERROR   | [x] Expected 1 newline at end of file; 0 found
------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------

For these applications, we need a project where, in at least the branch used for the application, most of the commits (if not all the commits) have been done from the user who applies.
The purpose of these applications is reviewing a project to understand what the user who applies understands about writing secure code that follows the Drupal coding standards and correctly uses the Drupal API. A project without code committed by the user who applies cannot be used for these applications, especially when the user doesn't have the permission to commit code.

I see commits from Luis Ruiz.

Hello vishal.kadam

Sorry for the missing work. I have already fix the problems with coding standards. Thank you very much for your review.

Hello apaderno, thank you for your comment, but these commit are done by me. The cause of the incongruity is the PC from I did the commit. Ones were done from my work PC and the another from my personal PC. The configuration of the user in the PC's are different but the Gitlab user is the same.

If you check my commits here https://git.drupalcode.org/users/Peidro/activity you wil see that are the same did by Luis Ruiz, Luis Peidro and lpeidro. In fact my complete name is "Luis Ruiz Peidro".

I changed the issue status to Needs review.

The commit that resolve the problems of coding standard is https://git.drupalcode.org/project/xray_audit/-/commit/e57111674e37ffff9...

Thanks.

Hello Luis,

I have reviewed the changes, and everything looks fine to me.

Let’s wait for other reviewers to take a look, and if everything goes fine, you will get the role.

Thanks

Hi @lpeidro,
This module works for me. I review this module manually and did not find any error.

Thanks.

• The following points are just a start and don't necessarily encompass all of the changes that may be necessary
• A specific point may just be an example and may apply in other places
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

src/Controller/XrayAuditGroupsController.php

    usort($groupDefinitions, fn($a, $b) => ($a['sort'] ?? 0) <=> ($b['sort'] ?? 0));

Drupal 9 still requires PHP 7.3. A module that uses PHP 7.4 or PHP 8 features should explicitly require that PHP version, or require Drupal 10.x, which requires PHP 8.1 or higher versions.

src/Plugin/tasks/DisplaysGroup/XrayAuditEntityDisplaysPlugin.php

  /**
   * Entity type manager.
   *
   * @var \Drupal\xray_audit\Services\DisplayModesEntityArchitectureInterface
   */
  protected $extractorDisplayModes;

  /**
   * Constructs a \Drupal\Component\Plugin\PluginBase object.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   * @param string $plugin_id
   *   The plugin_id for the plugin instance.
   * @param mixed $plugin_definition
   *   The plugin implementation definition.
   * @param \Drupal\xray_audit\Services\DisplayModesEntityArchitectureInterface $extractorDisplayModes
   *   Entity type manager.
   */

Both those comments contain wrong information: What described as entity type manager is not the entity type manager. The constructor is not for the \Drupal\Component\Plugin\PluginBase class.

src/Plugin/tasks/DisplaysGroup/XrayAuditEntityDisplaysPlugin.php

 /**
   * {@inheritdoc}
   */
  public function buildDataPresentationForCli(array $data, string $operation = NULL) {
    // @todo Implement buildDataPresentationForCli() method.
    return '';
  }

The code used for these applications is supposed to be complete. It should not contain methods that returns an empty string because the method has not been yet implemented.

  /**
   * Create static method.
   *
   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
   *   Container.
   * @param array $configuration
   *   Configuration.
   * @param string $plugin_id
   *   Plugin id.
   * @param mixed $plugin_definition
   *   Plugin definition.
   *
   * @return static
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {

Methods defined in an interface or parent class are documented with {@inheritdoc}.

    return [
      '#theme' => 'table',
      '#header' => ['Bundle', 'Displays', 'Displays Count'],
      '#rows' => $rows_current_data,
      '#weight' => 20,
    ];

Strings shown in the user interface need to be translatable.

src/Plugin/XrayAuditGroupPluginManager.php

  /**
   * Constructs XrayAuditGroupPluginManager object.
   *

The class name needs to include the full namespace.

Hello apaderno:

Thank you very much for your excellent work. I will comment on your observations:

About the compatibility with PHP 7.3: I used "phpcompatibility/php-compatibility" to ensure that the modifications are in the commit: https://git.drupalcode.org/project/xray_audit/-/commit/77132e82ec7dbdcec...

About comments in code: I review the comments and I think that I fixed all the potential problems. I remove the empty methods and I am going to create an Issue in the module to develop those features: