Drupal Coding Standards Issues | phpcs

I have fixed all phpcs issues apart from .module file issues as I was not sure how to change the static variables defined there. Marking this as `Needs Work` so that someone can pick that up and complete this.

Fixed phpcs issues applied the patch please review.

Applying patch #6, but still shows the following error

FILE: ...ues-9.5.x-dev\modules\image_widget_crop\image_widget_crop.module
----------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
  8 | WARNING | Global constants should not be used, move it to a
    |         | class or interface
  9 | WARNING | Global constants should not be used, move it to a
    |         | class or interface
 34 | ERROR   | All functions defined in a module file must be
    |         | prefixed with the module's name, found
    |         | "image_widget_cropfield_widget_info_alter" but
    |         | expected
    |         | "image_widget_crop_image_widget_cropfield_widget_info_alter"

Applying patch for address remaining errors.

Checked #10 patch it applied smoothly and remove all PHPCS issue now great work by @sahil.goyal . so move to RTBC

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

-   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $configFactory

Code can use a local variable named $config_factory. (A method parameter is still a local variable.)

Variables should be named using lowercase, and words should be separated either with uppercase characters (example: $lowerCamelCase) or with an underscore (example: $snake_case). Be consistent; do not mix camelCase and snake_case variable naming inside a file.

+   * @param \Drupal\Core\Entity\EntityTypeManager $entityTypeManager
+   *   The entity type manager.
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity
+   *   The Entity type manager service.
    */
-  public function __construct(ConfigFactoryInterface $config_factory, FileUsageInterface $file_usage, ImageWidgetCropInterface $iwc_manager) {
-    parent::__construct($config_factory);
+  public function __construct(ConfigFactoryInterface $configFactory, FileUsageInterface $file_usage, ImageWidgetCropInterface $iwc_manager, EntityTypeManager $entityTypeManager, EntityTypeManagerInterface $entity) {
+    parent::__construct($configFactory);
     $this->settings = $this->config('image_widget_crop_examples.settings');
+    $this->entityTypeManager = $entityTypeManager;

As per Drupal coding standards:

[D]o not mix camelCase and snake_case variable naming inside a file

The patch addresses the #19 suggestions. Please review.

Hi AkashKumar07

I applied your patch 3333260-20.patch to my local and I confirmed that it fixes all the PHPCS issues.

Please look at the screenshot attached for your reference

Thank you

 /**
  * Implements hook_field_widget_info_alter().
  */
-function image_widget_cropfield_widget_info_alter(array &$info) {
+function image_widget_crop_widget_info_alter(array &$info) {

Since that is code in the image_widget_crop.module file and the hook is hook_field_widget_info_alter(), the correct function name is image_widget_crop_field_widget_info_alter(), not image_widget_crop_widget_info_alter().

-    $js = IMAGE_WIDGET_CROP_JS_CDN;
+    $js = 'https://cdnjs.cloudflare.com/ajax/libs/cropper/4.0.0/cropper.min.js';

There is nothing wrong in using a constant, even if it would be better to define it with const. Actually, the Drupal coding standards says:

In Drupal 8 and later, constants should be defined using the const PHP language keyword (instead of define()), because it is better for performance:

/**
 * Indicates that the item should be removed at the next general cache wipe.
 */
 const CACHE_TEMPORARY = -1;

Note that const does not work with PHP expressions. define() should be used when defining a constant conditionally or with a non-literal value:

if (!defined('MAINTENANCE_MODE')) {
  define('MAINTENANCE_MODE', 'error');
}

+   /**
+    * Constructs a CropWidgetForm object.
+    *

The class namespace is missing from the description.

+-   * @param \Drupal\Core\Image\ImageFactory $imageFactory
++   * @param \Drupal\Core\Image\ImageFactory $image_factory

That part is adding extra - and +.

+-  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, ImageFactory $imageFactory) {
++  public function __construct(EntityTypeManagerInterface $entity_type_manager, ConfigFactoryInterface $config_factory, ImageFactory $image_factory) {
+     $this->entityTypeManager = $entity_type_manager;
+     $this->cropStorage = $this->entityTypeManager->getStorage('crop');
+     $this->cropTypeStorage = $this->entityTypeManager->getStorage('crop_type');
+     $this->imageStyleStorage = $this->entityTypeManager->getStorage('image_style');
+     $this->fileStorage = $this->entityTypeManager->getStorage('file');
+     $this->imageWidgetCropSettings = $config_factory->get('image_widget_crop.settings');
+-    $this->imageFactory = $imageFactory;
++    $this->imageFactory = $image_factory;

Drupal coding standards allow to name a local variable $image_factory. It is the class properties that should be named $imageFactory.
Also, that change is still adding extra - and +.

+  /**
+   * Drupal\Core\Entity\EntityTypeManager definition.
+   *
+   * @var \Drupal\Core\Entity\EntityTypeManager
+   */
+  protected $entityTypeManager;

A property description does not repeat its type.

+  /**
+   * The storage handler class for files.
+   *
+   * @var \Drupal\file\FileStorage
+   */
+  private $fileStorage;

The file storage. is sufficient.

+   * @param \Drupal\Core\Entity\EntityTypeManager $entity_type_manager
+   *   The entity type manager.
+   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity
+   *   The Entity type manager service.
    */
-  public function __construct(ConfigFactoryInterface $config_factory, FileUsageInterface $file_usage, ImageWidgetCropInterface $iwc_manager) {
+  public function __construct(ConfigFactoryInterface $config_factory, FileUsageInterface $file_usage, ImageWidgetCropInterface $iwc_manager, EntityTypeManager $entity_type_manager, EntityTypeManagerInterface $entity) {
     parent::__construct($config_factory);
     $this->settings = $this->config('image_widget_crop_examples.settings');
+    $this->entityTypeManager = $entity_type_manager;
     $this->fileUsage = $file_usage;
     $this->imageWidgetCropManager = $iwc_manager;
+    $this->fileStorage = $entity->getStorage('file');
   }

Something is wrong in that definition. Why would a method get a \Drupal\Core\Entity\EntityTypeManager parameter and a \Drupal\Core\Entity\EntityTypeManagerInterface parameter, since EntityTypeManagerInterface is the interface implemented by the EntityTypeManager class?
By the name, $entity is an entity object. Entity classes do not implement EntityTypeManagerInterface.

-        $this->messenger()->addMessage($this->t('The crop "@cropType" was successfully updated for image "@filename".', ['@cropType' => $crop_type->label(), '@filename' => $this->fileStorage->load($field_value['file-id'])->getFilename()]));
+        $this->messenger()->addMessage($this->t('The crop "@cropType" was successfully updated for image "@filename".',
+        [
+          '@cropType' => $crop_type->label(),
+          '@filename' => $this->fileStorage->load($field_value['file-id'])->getFilename(),
+        ]));
       }

I would leave that code as it is, since it is simpler to understand.

-                // If file-id key is not available, set it same as parent elements target_id
+                // If file-id key is not available,
+                // set it same as parent elements target_id.

If the file ID key has not been set, set it to the parent elements target ID.

-                // Parse all value of a crop_wrapper element and get properties
-                // associate with her CropType.
+                // Parse all value of a crop_wrapper element and get properties.
                 foreach ($crop_element['crop_wrapper'] as $crop_type_name => $properties) {
                   $properties = $properties['crop_container']['values'];

I would rather remove that comment, since it is not true the code is parsing values. It is looping over an array, but that does not need any explanation: Every developer knows what foreach() does in PHP.

Addressed some phpcs issues

The report is not updated for the recent changes in the ruleset. It also reports errors that are not present in the project files.

Issue summary updated

Please review.

Hi @sakthi_dev,

Applied MR !15 successfully, but it threw multiple errors.

 image_widget_crop git:(c42c4e6) curl https://git.drupalcode.org/project/image_widget_crop/-/merge_requests/15.diff | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 17786    0 17786    0     0  32933      0 --:--:-- --:--:-- --:--:-- 33621
patching file image_widget_crop.module
patching file image_widget_crop.services.yml
patching file modules/image_widget_crop_examples/src/Form/ImageWidgetCropExamplesForm.php
patching file src/Element/ImageCrop.php
patching file src/Form/CropWidgetForm.php
patching file src/ImageWidgetCropManager.php
patching file src/Plugin/Field/FieldWidget/ImageCropWidget.php
patching file tests/src/FunctionalJavascript/ImageWidgetCropTest.php
➜  image_widget_crop git:(c42c4e6) ✗ cd ..
➜  contrib git:(master) ✗ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig image_widget_crop

FILE: /Users/PrometInterns/Demo-site/drupal-orgissue-v9/web/modules/contrib/image_widget_crop/image_widget_crop.module
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  8 | WARNING | Global constants should not be used, move it to a class or interface
  9 | WARNING | Global constants should not be used, move it to a class or interface
 34 | ERROR   | All functions defined in a module file must be prefixed with the module's name, found "image_widget_cropfield_widget_info_alter" but expected
    |         | "image_widget_crop_image_widget_cropfield_widget_info_alter"
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /Users/PrometInterns/Demo-site/drupal-orgissue-v9/web/modules/contrib/image_widget_crop/modules/image_widget_crop_examples/src/Form/ImageWidgetCropExamplesForm.php
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 55 | ERROR | Missing short description in doc comment
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /Users/PrometInterns/Demo-site/drupal-orgissue-v9/web/modules/contrib/image_widget_crop/src/Form/CropWidgetForm.php
---------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------------
 10 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Extension\ModuleHandlerInterface.
---------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------------


FILE: /Users/PrometInterns/Demo-site/drupal-orgissue-v9/web/modules/contrib/image_widget_crop/src/ImageWidgetCropInterface.php
------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------
 58 | ERROR | [x] Multi-line function declarations must have a trailing comma after the last parameter
------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------

Time: 1.02 secs; Memory: 14MB

Kindly check.

Thanks,
Jake

Balu Ertl → made their first commit to this issue’s fork.

• It took more time to dig it up than expected, but finally, PHPUnit tests are working again after not being run for 1,5 years ago → .
• PHPCS, Stylelint, and composer-lint are all green.
• I didn't touch eslint as JavaScript falls out of my expertise. Cspell and PHPStan also could be improved, but they should be good to go for now, I think.
• @apaderno → 's comments went through and fixed those which was still unresolved.

I scanned through the changes and didn't seen anything major. If there are any left-over concerns, I would advocate for getting the Gitlab and test-changes in first, and then creating follow-up issues for the various linters. I just spent about half an hour hunting down tests breaking in the module because I wanted to try to get 🐛 Crop widget not adjusting crop box when crop is applied Needs work over the last hump by writing a test. I first found out tests haven't been running since there is no Gitlab integration and then I found this issue...