Fix the issues reported by phpcs

Hello,

I've reviwed the #2 patch but after applying the patch facing one issue.

---------------------------------------------------------------------------------------------------
FILE: /home/drupal/d9-open/visualization/src/Plugin/visualization/handler/HighchartsHandler.php
---------------------------------------------------------------------------------------------------
109 | ERROR | The array declaration extends to column 138 (the limit is 80). The array content should be split up over multiple lines
---------------------------------------------------------------------------------------------------

I have created a patch to fix this issue.
Kindly review the patch.

Hi, I am going to review the patch. :)

No issues were found in the #5 patch after being applied. I looked again for code standard issues on the module, but none were found—everything working properly.

#5 patch appied successfully and no phpcs issues found. Created MR

The issue summary should always describe what the issue is trying to fix and, in the case, of coding standards issues, report which command has been used, which arguments have been used, and which report that command shown.

Hey,
This module not compatible wth drupal 10 ?!
They are a anthor module like this, for migration site from 7 to 10, please
Thanks

Updating the IS

patch #5 applied successfully compatible with my version sharing the SS.

Since the merge request !3 is the last created, I am going to review that.

+  /**
+   * Construct.
+   */
   public function __construct(ConfigFactory $config_factory, VisualizationHandlerManager $handler_manager) {

That is not the short description used for class constructors.
The description for the parameters is missing.

+  /**
+   * Form Id.
+   */
+  public function getFormId() {
     return 'visualization_settings_form';
   }
 

Since that is a method defined from the parent class, the documentation comment is different.

 /**
  * @file
- * Definition of Drupal\visualization\Plugin\VisualizationHandlerManager
+ * Definition of Drupal\visualization\Plugin\VisualizationHandlerManager.
  */

@file is not used for files containing a single class definition.

+  /**
+   * Name.
+   *
+   * @var name
+   */
   public $name = 'highcharts';
 
+  /**
+   * Added Javascript.
+   *
+   * @var addedJavascript
+   */
   protected $addedJavascript = FALSE;
+
+  /**
+   * Available.
+   *
+   * @var available
+   */
   protected $available = FALSE;

Short descriptions should not just repeat the property name.

+   *   Some default information is:
+   *   - title: the label of the chart
+   *   - type: the internal name of the type of the chart; default types are
+   *     line, pie and column. Specific charting libraries are welcome to
+   *     implement custom chart types as they please.
+   *   - fields: an associative array of internal field names and their labels.
+   *   - xAxis: options concerning the x-axis (if appropriate)
+   *   - yAxis: options concerning the y-axis (if appropriate)

The lines after the first one should be indented.

+   * @return array
+   *   A string that will be the body of the chart div.

If the returned value is a string, it cannot be an array.

-core: 8.x
-configure: visualization.settings_form
\ No newline at end of file
+core_version_requirement: ^8

core_version_requirement is recognized only starting from Drupal 8.8. If core: 8.x is replaced by core_version_requirement: ^8, the module is not installable on earlier Drupal versions.

 Drupal.behaviors.visualization_gva = {
-  attach: function(context) {
-    $.each($(".visualization-chart-gva", context).not(".visualization-processed"), function(idx, value) {
+  attach: function (context) {
+    $.each($(".visualization-chart-gva", context).not(".visualization-processed"), function (idx, value) {
       var chart_id = $(value).attr("id");
       var chart = drupalSettings.visualization[chart_id];

The report shown in the issue summary does not show any issue for JavaScript files. Either the shown report is not complete, or those changes are off-topic.

addressed all the suggestive coding standards and warnings in #16.
only last suggestion is left,
As here task is to address PHPCS issues, which are related to the PHP code standards used in the project.

Regarding the JavaScript files, the report shown in the issue summary does not indicate any issues related to those files. Therefore, consider those changes as off-topic for current task of addressing PHPCS issues.
Please review.

Thanks @apaderno for addressing the issues.

I ran phpcs --standard="Drupal,DrupalPractice" --extensions=php,module,inc,install,test,profile,theme,info,txt,md,yml ./visualization and I got a longer report than what previously shown.

I updated the issue summary.

Please, when opening these issues, show the full report, not only part of it as it is more convenient for the user who is going to provide a patch or create a MR.

Let's start with those issues. I would leave the ones about JavaScript code for a different issue.

Hi @apaderno, when i run phpcs --standard="Drupal,DrupalPractice" --extensions=php,module,inc,install,test,profile,theme,info,txt,md,yml ./visualization i can only seen a single issue, not a list of as updated in IS. Attaching Sreenshot.

Cloning the 8.x-1.x branch with git clone --branch '8.x-1.x' https://git.drupalcode.org/project/visualization.git ./visualization
and then executing phpcs --standard="Drupal,DrupalPractice" --extensions=php,module,inc,install,test,profile,theme,info,txt,md,yml ./visualization gives the report shown in the issue summary. I tried again, and the report is not just for a single line.

Please review. Addressed as per comment on MR.

Hi @sakthi_dev & @silvi.addweb,

The changes you made on MR !3 was applied smoothly, however, one error still reported, please see below:

visualization git:(8.x-1.x) curl https://git.drupalcode.org/project/visualization/-/merge_requests/3.diff | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 32383    0 32383    0     0  36151      0 --:--:-- --:--:-- --:--:-- 36590
patching file js/gva.js
patching file js/highcharts.js
patching file js/visualization.js
patching file src/Form/VisualizationSettingsForm.php
patching file src/Plugin/VisualizationHandlerManager.php
patching file src/Plugin/views/style/Visualization.php
patching file src/Plugin/visualization/handler/GoogleVisualizationAPIHandler.php
patching file src/Plugin/visualization/handler/HighchartsHandler.php
patching file src/VisualizationHandlerInterface.php
patching file visualization.api.php
patching file visualization.info.yml
patching file visualization.links.menu.yml
patching file visualization.module
patching file visualization.theme.inc
➜  visualization git:(8.x-1.x) ✗ cd ..
➜  contrib git:(main) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig visualization

FILE: ...web/modules/contrib/visualization/src/Plugin/views/style/Visualization.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 175 | ERROR | Public method name "Visualization::build_sort" is not in
     |       | lowerCamel format
--------------------------------------------------------------------------------

Time: 279ms; Memory: 10MB

Kindly check

Thanks,
Jake

MR Raised