Fix the issues reported by phpcs

Fixed the PHPCS issue. Please review.

-  // TODO: This code only covers configurable fields, handle base table fields.
+  // @todo Fix problem configurable fields, handle base table fields here.

The existing sentence is already correct: That @todo is about handling base table fields, not fixing problems with configurable fields. What needs to be changed is replacing TODO: with @todo.

-   * @todo: add more tests, particularly around max/min values.
+   * @todo Fix problem add more tests, particularly around max/min values here.
    */

There is no need to add Fix problem; Fix problem add more tests does not make grammatically sense. Add more tests is correct.

 /**
- * Trait GeofieldBoundaryHandlerTrait.
+ * Trait for GeofieldBoundaryHandlerTrait.
  */

The description repeats the trait name, like the existing description. It needs to describe what the trait does.

   /**
    * {@inheritdoc}
    */
-  public $no_operator = TRUE;
+  public $noOperator = TRUE;

That is a property inherited from a parent class, for which GeofieldRectBoundaryFilter is just setting a different default value. That property name cannot be changed, if the parent class does not change it.

Kindly review patch

The last submitted patch, 7: 3360474-7.patch, failed testing. View results →

-     // Generate the WKT version of the point geometry: 'POINT (-73.985664 41.748441)'
+     // Generate the WKT version of the point geometry: 
+     'POINT (-73.985664 41.748441)'

In the existing code, 'POINT (-73.985664 41.748441)' is part of a comment. In the changed code, 'POINT (-73.985664 41.748441)' is part of PHP code.

-     'value' => 'POLYGON((-73.98411932014324 40.754779803566606,-73.98502054237224 40.75354445673964,-73.98186626457073 40.75221155678824,-73.98092212699748 40.75344692838096,-73.98411932014324 40.754779803566606))',
+     'value' => 'POLYGON((-73.98411932014324 40.754779803566606,
+     -73.98502054237224 40.75354445673964,-73.98186626457073 40.75221155678824,
+     -73.98092212699748 75344692838096,-73.98411932014324 40.754779803566606))',
      ];

That change is not correct, as it introduces new lines characters in a literal string that did not contain them.

-define('GEOFIELD_TYPE_MULTIPOINT', 'MULTIPOINT');
+GeoFieldsInterface::GEOFIELD_TYPE_MULTIPOINT;

That is wrong change: define('GEOFIELD_TYPE_MULTIPOINT', 'MULTIPOINT'); defines a constant, but GeoFieldsInterface::GEOFIELD_TYPE_MULTIPOINT is using a constant, not defining it.

-(function(geolocation, $){
-  if (geolocation) return;
+(function (geolocation, $) {
+  if (geolocation) { return;
+  }

The code is not correctly formatted: return must go on a new line.

-    if (cache) callback(cache);
+    if (cache) { callback(cache);
+    }

That code is not correctly formatted too.

         longitudeSpan.text(lon);
-        return false;
+        return FALSE;

In JavaScript code, Drupal core uses false, not FALSE.

-        $form_state->setError($element[$key], t('@title: @component_title is not valid.', ['@title' => $error_label, '@component_title' => $component['title']]));
+        $form_state->setError($element[$key], t('@title: @component_title is not valid.',
+        ['@title' => $error_label, '@component_title' => $component['title']]));
       }

That code is not correctly formatted.

-      $element['#element_validate'] = [[get_class($this), 'validateGeofieldGeometryText']];
+      $element['#element_validate'] = [[get_class($this),
+        'validateGeofieldGeometryText',
+      ],
+      ];

That code is not formatted correctly.

-      $values[$delta]['value'] = $this->geofieldBackendValue($this->wktGenerator->wktBuildPoint([trim($components['lon']), trim($components['lat'])]));
+      $values[$delta]['value'] = $this->geofieldBackendValue($this->wktGenerator->wktBuildPoint(
+      [trim($components['lon']), trim($components['lat'])]));

That code is not correctly formatted.

-      throw new InvalidPointException($this->t('@proximity_handler reports Invalid Point coordinates', [
-        '@proximity_handler' => get_class($this),
-      ]));
+      throw new InvalidPointException("@proximity_handler reports Invalid Point coordinates", get_class($this)
+      );

That is a wrong change: The changed code is not equivalent to the existing code.

-      throw new ProximityUnavailableException($this->t('@proximity_handler not able to calculate valid Proximity value', [
-        '@proximity_handler' => get_class($this),
-      ]));
+      throw new ProximityUnavailableException("@proximity_handler not able to calculate valid Proximity value",
+       get_class($this,
+      ));

To make clearer what is wrong in that change: The existing code passes a single argument to the ProximityUnavailableException constructor, while the changed code passes two arguments to the same constructor.

-   * @todo: add more tests, particularly around max/min values.
+   * @todo add more tests, particularly around max/min values.

What follows @todo is a sentence: It starts with a capitalized word and it ends with a period (preferable), a question mark, or an exclamation point.

(Furthermore, changing the code to follow the Drupal coding standards is a task.)

zniki.ru → made their first commit to this issue’s fork.

MR updated and ready for review.
Not sure how to fix code style violations in README.md.

Please review.

Hey folk here ... should/could we merge this?
I added some commits ...
Isn't enough for marking this as RWTBC and deploying this into dev branch?

Thanks for update, I provide some comments on MR, please check.
I added commit to fix use statement sort.

Run phpcs on latest changes:

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

FILE: /geofield/geofield.module
-------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 13 WARNINGS AFFECTING 13 LINES
-------------------------------------------------------------------------------------
 21 | WARNING | Global constants should not be used, move it to a class or interface
 28 | WARNING | Global constants should not be used, move it to a class or interface
 35 | WARNING | Global constants should not be used, move it to a class or interface
 42 | WARNING | Global constants should not be used, move it to a class or interface
 49 | WARNING | Global constants should not be used, move it to a class or interface
 56 | WARNING | Global constants should not be used, move it to a class or interface
 61 | WARNING | Global constants should not be used, move it to a class or interface
 66 | WARNING | Global constants should not be used, move it to a class or interface
 71 | WARNING | Global constants should not be used, move it to a class or interface
 76 | WARNING | Global constants should not be used, move it to a class or interface
 81 | WARNING | Global constants should not be used, move it to a class or interface
 86 | WARNING | Global constants should not be used, move it to a class or interface
 91 | WARNING | Global constants should not be used, move it to a class or interface
-------------------------------------------------------------------------------------

Update issue summary. Add phpcs result on latest changes from 8.x-1.x.

Folks ... thanks for the good contribution here, but let's stop it here, also because this is already set as "Needs review".
I am going to QA and Review this MR as it is now, and not expect further commits / changes, because indeed we don't need to be too picky and solve every single minor warning on PHPCs in the Geofield module ... because that's not really needed (many, if not even all, of the Drupal core modules are 100% compliant with drupal & php coding standards).

Also I am not going to refactor all the constants now defined in the gefoield.module file, because that is working and has been working fine, and I don't want to risk any regression, also in the other modules that have Geofield as their dependency (Geofield Map, Leaflet, Geocoder ... ).

I will come back to this shortly with my Tests & QA outcomes.

Thanks for your feedback, as an option it's possible to create phpcs.xml.dist and mute some of the changes, that we don't want to happen.
I have an idea to fix all phpcs violations and enable gitlab ci intergration (issue 3402400 📌 Adopt GitlabCi Needs review ), to prevent future violations.

Thanks to you @zniki.ru ...
Personally I am not sure we need to silent any phpcs warning on the Geofield module, and sincerely I don't think the module needs it.
Simply we need to discriminate what we strictly need to fix and what not.
I would keep the phpcs warnings (they can still help) and eventually fix the ones not already coped here, in the future ...
For this MR I think that we added enough fixes.

Ok ... good job! I tested all this in a real project env of mine, and no issues were logged from Geofield module.
So, let's merge into the 8.x-1.x-dev branch.
Thanks everybody for the great contribution.

Thanks a lot for merging these changes to code base.
Can you please check credits for this issue.
According to documentation → I think that I, nitin_lama, Shank115, apaderno should also get credits on resolving this issue.

Fair enough. Pushed another empty commit in this, giving credits to all of you/them.

Truly, zniki.ru was referring to the credits listed in Credit given to itamair, sakthi_dev at Specbee for Drupal India Association, arti_parmar at Smashing Infolabs Pvt. Ltd. Those do not require commits to be changed.

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