Problem/Motivation

Currently there's some inconsistency and violations

ds$ phpcs --standard=Drupal --report=summary .

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
...skilld/ds/drush/example_layout/example_layout.inc  0       2
/home/andypost/www/skilld/ds/includes/field_ui.inc    1       4
/home/andypost/www/skilld/ds/src/Ds.php               1       0
.../andypost/www/skilld/ds/src/Form/SettingsForm.php  1       0
...andypost/www/skilld/ds/src/Form/FieldFormBase.php  0       1
...andypost/www/skilld/ds/src/Form/CopyFieldForm.php  1       0
...ndypost/www/skilld/ds/src/Plugin/DsField/Date.php  0       1
...skilld/ds/src/Plugin/DsField/DsFieldInterface.php  1       0
...dypost/www/skilld/ds/src/Plugin/DsField/Field.php  0       1
...andypost/www/skilld/ds/src/Annotation/DsField.php  2       0
----------------------------------------------------------------------
A TOTAL OF 7 ERRORS AND 9 WARNINGS WERE FOUND IN 10 FILES
----------------------------------------------------------------------


ds$ phpcs --standard=DrupalPractice --report=summary .

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
.../modules/ds_test/src/Plugin/Block/DsTestBlock.php  0       1
...s/ds_extras/src/Controller/DsExtrasController.php  0       2
/home/andypost/www/skilld/ds/includes/field_ui.inc    0       5
.../andypost/www/skilld/ds/src/Form/SettingsForm.php  0       2
...andypost/www/skilld/ds/src/Form/FieldFormBase.php  0       1
...andypost/www/skilld/ds/src/Form/EmergencyForm.php  0       5
...ypost/www/skilld/ds/src/Form/ChangeLayoutForm.php  0       3
...e/andypost/www/skilld/ds/src/Form/ClassesForm.php  0       1
...ost/www/skilld/ds/src/Controller/DsController.php  0       1
----------------------------------------------------------------------
A TOTAL OF 0 ERRORS AND 21 WARNINGS WERE FOUND IN 9 FILES
----------------------------------------------------------------------

Proposed resolution

Patch 17 contains a number of straightforward fixes. (But it has not passed tests, as "no tests found".)
Patch 18 contains a couple of fixes that may or may not be complete - depending on whether the methods that have been renamed are actually used anywhere.
Patch 23 contains a tentative attempt to use dependency injection as recommended by the DrupalPractice code sniffer, but again the testbot concluded "no tests found".

Remaining tasks

clean-up the rest

Issue fork ds-2838343

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
20.95 KB

After phpcbf

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
/work/ds.module                                       0       1
/work/drush/example_layout/example_layout.inc         1       2
/work/drush/ds.drush.inc                              1       0
/work/tests/modules/ds_test/ds_test.module            1       1
.../modules/ds_test/src/Plugin/Block/DsTestBlock.php  1       0
...k/modules/ds_switch_view_mode/src/Permissions.php  1       0
/work/modules/ds_extras/ds_extras.install             2       0
/work/modules/ds_extras/ds_extras.module              3       0
/work/includes/field_ui.inc                           11      1
/work/ds.api.php                                      13      0
/work/src/Ds.php                                      2       0
/work/src/Tests/DsTestTrait.php                       2       0
/work/src/Form/SettingsForm.php                       1       0
/work/src/Form/FieldFormBase.php                      0       1
/work/src/Form/CopyFieldForm.php                      1       0
/work/src/Plugin/DsField/Date.php                     0       1
/work/src/Plugin/DsField/DsFieldInterface.php         2       0
/work/src/Plugin/DsField/Field.php                    0       1
/work/src/Annotation/DsFieldTemplate.php              1       0
/work/src/Annotation/DsField.php                      3       0
----------------------------------------------------------------------
A TOTAL OF 46 ERRORS AND 8 WARNINGS WERE FOUND IN 20 FILES
----------------------------------------------------------------------
aspilicious’s picture

Issue tags: +drupalmountaincamp
martin_q’s picture

FileSize
20.36 KB

I'm reviewing this at MountainCamp 2017 and will gradually grow the revised patch; posting partial versions in case I run out of time.

diff --git a/drush/example_layout/example_layout.inc b/drush/example_layout/example_layout.inc
index 54ed23a..c22ea6e 100644
--- a/drush/example_layout/example_layout.inc
+++ b/drush/example_layout/example_layout.inc
@@ -5,6 +5,9 @@
  * Display Suite example layout configuration.
  */
 
+/**
+ *
+ */

Adding an empty doxygen block may pass the code sniffer but actual documentation would be more helpful.

     // Uncomment if you want to include a preview for this layout (example_layout.png).
-    // 'image' => TRUE,
+    // 'image' => TRUE,.

Whilst most comments should end with a period/full-stop, putting one here would break the code if someone were to follow the instructions and uncomment that line.

diff --git a/ds.api.php b/ds.api.php
index d24d57b..6ac7047 100644
--- a/ds.api.php
+++ b/ds.api.php
@@ -78,7 +78,7 @@ function hook_ds_layout_settings_alter($record, \Drupal\Core\Form\FormStateInter
  *   - entity
  *   - entity_type
  *   - bundle
- *   - view_mode
+ *   - view_mode.

The coding standards don't insist on a list ending with a period/full-stop and when the list items are single-word values, I'd suggest that none of them need one.

diff --git a/modules/ds_extras/ds_extras.install b/modules/ds_extras/ds_extras.install
index 30e59ca..9ed1cac 100644
--- a/modules/ds_extras/ds_extras.install
+++ b/modules/ds_extras/ds_extras.install
@@ -1,20 +1,28 @@
 <?php
 
 /**
+ * @file
+ */
+
+/**
  * Implements hook_install().
  */
+
+/**
+ *
+ */
 function ds_extras_install() {
   module_set_weight('ds_extras', 2);
 }

The @file docblock would be better with some actual content, and the additional empty docblock above ds_extras_install is definitely wrong, as it already had the correct docblock which has now been pushed out of the way.

martin_q’s picture

FileSize
20.72 KB
diff --git a/modules/ds_extras/ds_extras.module b/modules/ds_extras/ds_extras.module
index 1cde876..fffafbb 100644
--- a/modules/ds_extras/ds_extras.module
+++ b/modules/ds_extras/ds_extras.module
@@ -56,7 +56,7 @@ function ds_extras_form_entity_view_display_edit_form_alter(&$form, FormStateInt
   // a view mode which is not equal to default.
   $mode = $form_state->getFormObject()->getEntity()->getMode();
   if (isset($form['#ds_layout']) && $mode != 'default' && \Drupal::config('ds_extras.settings')
-      ->get('region_to_block')
+    ->get('region_to_block')
   ) {

A closing ')' at the beginning of a line should have a corresponding '(' at the start of a line, which it doesn't here, but this is not recommended practice anyway with if conditions. So the whole if statement should really be one line:

  if (isset($form['#ds_layout']) && $mode != 'default' && \Drupal::config('ds_extras.settings')->get('region_to_block')) {

This is long, though technically allowed - but perhaps an extra level of nested if statement would be useful?

  if (isset($form['#ds_layout']) && $mode != 'default') {
    if (\Drupal::config('ds_extras.settings')->get('region_to_block')) {

I'll leave it at the single-line version for now, though.

--- a/tests/modules/ds_test/src/Plugin/Block/DsTestBlock.php
+++ b/tests/modules/ds_test/src/Plugin/Block/DsTestBlock.php
@@ -35,6 +35,9 @@ class DsTestBlock extends BlockBase {
     return $build;
   }
 
+  /**
+   *
+   */
   public function getCacheMaxAge() {
     return 0;
   }

Suggest actually putting something in the docblock: Ensure that this object is not cached.

Status: Needs review » Needs work

The last submitted patch, 5: 2838343-5.patch, failed testing.

martin_q’s picture

Status: Needs work » Needs review
FileSize
32.32 KB

Fixed bug in previous patch.

Also addressed a number of other issues reported by code sniffer.

The remaining errors are as follows - in many cases I think it is debatable whether they are really errors in these specific instances:

FILE: .../www/drupal-8/modules/ds/drush/example_layout/example_layout.inc
----------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
 18 | WARNING | [ ] Line exceeds 80 characters; contains 88
    |         |     characters
 20 | WARNING | [ ] Line exceeds 80 characters; contains 87
    |         |     characters
 21 | ERROR   | [x] Inline comments must end in full-stops,
    |         |     exclamation marks, colons, question marks, or
    |         |     closing parentheses
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/includes/field_ui.inc
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 905 | WARNING | Do not concatenate strings to translatable strings,
     |         | they should be part of the t() argument and you
     |         | should use placeholders
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/ds.api.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 81 | ERROR | [x] Parameter comment must end with a full stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...KM/www/drupal-8/modules/ds/tests/modules/ds_test/ds_test.install
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 13 | ERROR | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/ds.module
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 608 | WARNING | Only string literals should be passed to t() where
     |         | possible
----------------------------------------------------------------------


FILE: ...mq/KM/www/drupal-8/modules/ds/modules/ds_extras/ds_extras.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 325 | ERROR | [x] Parameter comment must end with a full stop
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...pal-8/modules/ds/modules/ds_switch_view_mode/src/Permissions.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 17 | ERROR | PHP4 style constructors are not allowed; use
    |       | "__construct()" instead
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/src/Plugin/DsField/Field.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 25 | WARNING | Only string literals should be passed to t() where
    |         | possible
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/src/Plugin/DsField/Date.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 77 | WARNING | Only string literals should be passed to t() where
    |         | possible
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/src/Annotation/DsField.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 35 | ERROR | Class property $entity_type should use lowerCamel
    |       | naming without underscores
 44 | ERROR | Class property $ui_limit should use lowerCamel naming
    |       | without underscores
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/src/Form/FieldFormBase.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 142 | WARNING | Avoid backslash escaping in translatable strings
     |         | when possible, use "" quotes instead
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/src/Form/CopyFieldForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 20 | ERROR | Public method name "CopyFieldForm::getFormID" is not in
    |       | lowerCamel format
----------------------------------------------------------------------


FILE: /home/mq/KM/www/drupal-8/modules/ds/src/Form/SettingsForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 123 | ERROR | Concatenating translatable strings is not allowed, use
     |       | placeholders instead and only one string literal
----------------------------------------------------------------------

  • aspilicious committed 964352f on 8.x-3.x authored by martin_q
    Issue #2838343 by martin_q, andypost: Clean-up codebase according...

  • aspilicious committed 3fbfba9 on 8.x-2.x authored by martin_q
    Issue #2838343 by martin_q, andypost: Clean-up codebase according...
aspilicious’s picture

Thnx for all your work, I reviewed the patch and committed it.
Maybe you can fix some off the remaining warnings. :)

andypost’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev
Status: Needs review » Needs work

I'm sure at least

FILE: ...pal-8/modules/ds/modules/ds_switch_view_mode/src/Permissions.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
17 | ERROR | PHP4 style constructors are not allowed; use
| | "__construct()" instead

should be fixed

andypost’s picture

Status: Needs work » Needs review
FileSize
2.74 KB

we mostly everywhere using "get"

andypost’s picture

Drupal practice shows more, fixed most obvious

Remains are

ds$ phpcs --standard=DrupalPractice .

FILE: ...skilld/ds/tests/modules/ds_test/src/Plugin/Block/DsTestBlock.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 26 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...killd/ds/modules/ds_extras/src/Controller/DsExtrasController.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 37 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
 61 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
----------------------------------------------------------------------


FILE: /home/andypost/www/skilld/ds/includes/field_ui.inc
----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
  284 | WARNING | Unused variable $layout_plugin.
  285 | WARNING | Unused variable $layout_form.
  291 | WARNING | Unused variable $layout_form_state.
 1118 | WARNING | Variable $displays is undefined.
 1120 | WARNING | Variable $displays is undefined.
----------------------------------------------------------------------


FILE: /home/andypost/www/skilld/ds/src/Form/SettingsForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 195 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 197 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: /home/andypost/www/skilld/ds/src/Form/FieldFormBase.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 243 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: /home/andypost/www/skilld/ds/src/Form/EmergencyForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
 149 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 150 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 154 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 168 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 169 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: /home/andypost/www/skilld/ds/src/Form/ChangeLayoutForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
  77 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
  96 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
 200 | WARNING | \Drupal calls should be avoided in classes, use
     |         | dependency injection instead
----------------------------------------------------------------------


FILE: /home/andypost/www/skilld/ds/src/Form/ClassesForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 34 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
----------------------------------------------------------------------


FILE: /home/andypost/www/skilld/ds/src/Controller/DsController.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 54 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
----------------------------------------------------------------------

Time: 553ms; Memory: 18Mb
andypost’s picture

Issue summary: View changes

updated IS

Status: Needs review » Needs work

The last submitted patch, 13: clean_up_codebase-2838343-13.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
7.05 KB

proper patch

martin_q’s picture

FileSize
17.97 KB
11.06 KB

Some further issues addressed.

martin_q’s picture

Meanwhile, per the remaining error messages, I renamed some methods but I am not sure about tracking down where the old method names are called so that I can change them there as well. If the testbot fails this patch then that will perhaps show some of the places where they are called but it doesn't seem a very safe approach. I don't know my way around sufficiently to be sure on this.

This patch does not contain the other changes from clean_up_codebase-2838343-17.patch so it does not replace that patch.

martin_q’s picture

FILE: /home/mq/KM/www/drupal-8/modules/ds/includes/field_ui.inc
----------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------
  284 | WARNING | Unused variable $layout_plugin.
  285 | WARNING | Unused variable $layout_form.
  291 | WARNING | Unused variable $layout_form_state.
 1122 | WARNING | Variable $displays is undefined.
 1124 | WARNING | Variable $displays is undefined.
----------------------------------------------------------------------

The first three are because some code is currently commented out, pending a TODO.
The latter two look like a bug to me, but I'm not sure what the original intention was.

FILE: ...odules/ds/tests/modules/ds_test/src/Plugin/Block/DsTestBlock.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 26 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
----------------------------------------------------------------------


FILE: ...s/ds/tests/modules/ds_test/src/Plugin/Block/DsCacheTestBlock.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 24 | WARNING | \Drupal calls should be avoided in classes, use
    |         | dependency injection instead
----------------------------------------------------------------------

I have attempted to implement dependency injection in these two files. Still very much on a learning curve with these so am submitting them for testing (or feedback) before I proceed further.

Status: Needs review » Needs work

The last submitted patch, 19: clean_up_codebase_dependency_injection-2838343-19.patch, failed testing.

martin_q’s picture

Status: Needs work » Needs review
FileSize
2.81 KB

Rerolled patch 19

Status: Needs review » Needs work

The last submitted patch, 21: clean_up_codebase_dependency_injection-2838343-21.patch, failed testing.

martin_q’s picture

Status: Needs work » Needs review
FileSize
2.8 KB

Rerolled 19/21 again.

martin_q’s picture

Issue summary: View changes

Updated issue summary with current status of patches 17, 18 and 23.

aspilicious’s picture

FileSize
21.8 KB

Ok in an effort to reduce the errors. New patch.

aspilicious’s picture

@martin_q I missed your patch. If this one gets in, could you reroll it on top of mine?

martin_q’s picture

Hi @aspilicious,
I ended up with three separate patches to address separate issues (see the issue summary). So which one are you referring to, or should (presumably) all three be rerolled on top of yours? I suspect your patch 25 does some of the same things as my patch 17. I came to a halt on this last month as I wasn't sure how to proceed.

aspilicious’s picture

FileSize
23.01 KB

  • aspilicious committed 5749ffc on 8.x-3.x
    Issue #2838343 by martin_q, andypost, aspilicious: Clean-up codebase...
andypost’s picture

andypost’s picture

FileSize
25.28 KB

Here's a lot of clean-up

- Added params to doc-blocks of interfaces
- Lots of code style fixes
- Refactored places to properly use entity api
- Fixed DI in several places

Status: Needs review » Needs work

The last submitted patch, 31: 2838343-31.patch, failed testing.

aspilicious’s picture

We can't change the interface (not even type hinting) as it is a BC break.

andypost’s picture

Status: Needs work » Needs review
FileSize
17.89 KB
44.39 KB

@aspilicious There's no changes in interfaces, just fixes for documentation

new patch
- fix test failure
- clean-up inline type-hints

andypost’s picture

FileSize
11.95 KB
54.76 KB

A bit more clean-up

andypost’s picture

aspilicious’s picture

Status: Needs review » Needs work

This needs a reroll after all the recent changes.

andypost’s picture

Status: Needs work » Needs review
FileSize
71.39 KB

Reroll, some parts are fixed + new ones

PHP CODE SNIFFER REPORT SUMMARY
----------------------------------------------------------------------
FILE                                                  ERRORS  WARNINGS
----------------------------------------------------------------------
/work/ds.module                                       2       5
/work/drush/example_layout/example_layout.inc         1       2
...les/ds_test/src/Plugin/Block/DsCacheTestBlock.php  0       1
.../modules/ds_test/src/Plugin/Block/DsTestBlock.php  0       1
...ds_extras/src/EventSubscriber/RouteSubscriber.php  0       1
...s/ds_extras/src/Controller/DsExtrasController.php  0       1
/work/includes/field_ui.inc                           2       6
/work/ds.install                                      0       1
/work/src/Form/SettingsForm.php                       1       2
/work/src/Form/FieldFormBase.php                      0       2
/work/src/Form/EmergencyForm.php                      0       4
/work/src/Form/ClassesForm.php                        0       1
/work/src/Plugin/DsField/Date.php                     0       1
/work/src/Plugin/DsField/DsFieldInterface.php         2       0
/work/src/Plugin/DsField/Field.php                    0       1
...ugin/DsFieldTemplate/DsFieldTemplateInterface.php  3       0
/work/src/Annotation/DsField.php                      2       0
/work/src/Controller/DsController.php                 0       1
----------------------------------------------------------------------
A TOTAL OF 13 ERRORS AND 30 WARNINGS WERE FOUND IN 18 FILES
----------------------------------------------------------------------
PHPCBF CAN FIX 6 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Status: Needs review » Needs work

The last submitted patch, 38: 2838343-38.patch, failed testing. View results

urvashi_vora’s picture

Status: Needs work » Needs review
FileSize
54.98 KB

Please review the patch. Thanks

Status: Needs review » Needs work

The last submitted patch, 40: 2838343-40.patch, failed testing. View results

swentel’s picture

Version: 8.x-3.x-dev » 5.0.x-dev
ayush.khare’s picture

Assigned: Unassigned » ayush.khare
ayush.khare’s picture

Assigned: ayush.khare » Unassigned
Status: Needs work » Needs review
FileSize
49.35 KB
50.98 KB

Added reroll diff for #40 and also removed some phpcs issues.

emarinho’s picture

Assigned: Unassigned » emarinho

I'll review it!

emarinho’s picture

Assigned: emarinho » Unassigned
Status: Needs review » Needs work

I've applied patch #44, but there still some work to do.
Listed some of the PHPCS errors and warnings that I've found.

FILE: /modules/contrib/ds/includes/field_ui.inc
---------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
---------------------------------------------------------------------------------------------------------------------------------------------
 295 | WARNING | Unused variable $layout_form_state.
 296 | WARNING | '@todo' should match the format '@todo Fix problem X here.'
 297 | WARNING | Line exceeds 80 characters; contains 85 characters
 997 | WARNING | Do not concatenate strings to translatable strings, they should be part of the t() argument and you should use placeholders
---------------------------------------------------------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/src/Ds.php
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 111 | ERROR | Missing parameter type
--------------------------------------------------------------------------


FILE: /modules/contrib/ds/src/Form/SettingsForm.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
-------------------------------------------------------------------------------------------------------------------------
 135 | ERROR   | Concatenating translatable strings is not allowed, use placeholders instead and only one string literal
 208 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 210 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/src/Form/FieldFormBase.php
-------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------------
 142 | WARNING | Avoid backslash escaping in translatable strings when possible, use "" quotes instead
 243 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/src/Plugin/DsField/DsFieldInterface.php
-------------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AND 1 WARNING AFFECTING 3 LINES
-------------------------------------------------------------------------------------------------------
  5 | WARNING | [x] Unused use statement
 31 | ERROR   | [ ] Type hint "array" missing for $settings
 44 | ERROR   | [ ] Type hint "array" missing for $form
-------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------


FILE: /home/emarinho/Contrib/install-dir/web/modules/contrib/ds/src/Plugin/DsField/Field.php
--------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------
 26 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/tests/modules/ds_test/src/Plugin/Block/DsTestBlock.php
----------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------
 26 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/modules/ds_extras/src/Controller/DsExtrasController.php
-----------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------
 42 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 69 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-----------------------------------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/includes/field_ui.inc
---------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
---------------------------------------------------------------------------------------------------------------------------------------------
 295 | WARNING | Unused variable $layout_form_state.
 296 | WARNING | '@todo' should match the format '@todo Fix problem X here.'
 297 | WARNING | Line exceeds 80 characters; contains 85 characters
 997 | WARNING | Do not concatenate strings to translatable strings, they should be part of the t() argument and you should use placeholders
---------------------------------------------------------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/src/Form/SettingsForm.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
-------------------------------------------------------------------------------------------------------------------------
 135 | ERROR   | Concatenating translatable strings is not allowed, use placeholders instead and only one string literal
 208 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 210 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/src/Form/FieldFormBase.php
-------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------------
 142 | WARNING | Avoid backslash escaping in translatable strings when possible, use "" quotes instead
 243 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-------------------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/src/Form/ChangeLayoutForm.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 5 WARNINGS AFFECTING 5 LINES
----------------------------------------------------------------------------------------------
 118 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 137 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 152 | WARNING | Unused variable $layout_options.
 165 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 168 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
----------------------------------------------------------------------------------------------


FILE: /modules/contrib/ds/src/Controller/DsController.php
--------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------
  55 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 145 | WARNING | EntityViewDisplay::load calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------------------------------------
emarinho’s picture

Assigned: Unassigned » emarinho

I'll work on this!

emarinho’s picture

Assigned: emarinho » Unassigned
Status: Needs work » Needs review

Fixed some of the CS and Dependecy Injections, please kindly review it. There are also some CS to fix:

FILE: /ds/includes/field_ui.inc
-------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------
 295 | WARNING | Unused variable $layout_form_state.
-------------------------------------------------------------------------------------


FILE: /ds/src/Form/SettingsForm.php
-----------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------
 164 | ERROR | Concatenating translatable strings is not allowed, use placeholders instead and only one string literal
-----------------------------------------------------------------------------------------------------------------------


FILE: /ds/src/Plugin/DsField/Field.php
--------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------
 26 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------------------


FILE: /ds/modules/ds_extras/src/Controller/DsExtrasController.php
-----------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------
 42 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 69 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
-----------------------------------------------------------------------------------------------------------------------


FILE: /ds/src/Form/SettingsForm.php
-----------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------
 164 | ERROR | Concatenating translatable strings is not allowed, use placeholders instead and only one string literal
-----------------------------------------------------------------------------------------------------------------------


FILE: /ds/src/Form/ChangeLayoutForm.php
---------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------
 179 | WARNING | Unused variable $layout_options.
---------------------------------------------------------------------------------------------


FILE: /ds/src/Controller/DsController.php
--------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------
  55 | WARNING | \Drupal calls should be avoided in classes, use dependency injection instead
 145 | WARNING | EntityViewDisplay::load calls should be avoided in classes, use dependency injection instead
--------------------------------------------------------------------------------------------------------------
LeoAlcci’s picture

Assigned: Unassigned » LeoAlcci

I'll review it!

LeoAlcci’s picture

Assigned: LeoAlcci » Unassigned

Thank you for the contribution, I manage to fixed PHPInt and fixed almost all the phpcs, the only thing left is DI and minimal things. Please review and keep with the good work =).

christyanpaim’s picture

Assigned: Unassigned » christyanpaim
christyanpaim’s picture

Assigned: christyanpaim » Unassigned
adaucyj’s picture

Assigned: Unassigned » adaucyj

I'll review it.

adaucyj’s picture

Status: Needs review » Needs work

I'm working on it.

adaucyj’s picture

Now remains the errors on test files. I'll keep working on it.

adaucyj’s picture

Assigned: adaucyj » Unassigned
Status: Needs work » Needs review

All PHPCS errors/warnings were cleaned up. The only ones left are regarding the issue #3223617 and will be fixed there.

  • swentel committed 60933a6 on 5.0.x authored by emarinho
    Issue #2838343 by andypost, martin_q, adaucyj, LeoAlcci, aspilicious,...
swentel’s picture

Status: Needs review » Fixed

Thank you everyone, merged! Woohoo!

swentel’s picture

Status: Fixed » Closed (fixed)