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
Comment | File | Size | Author |
---|---|---|---|
#44 | rerolldiff_40-44.txt | 50.98 KB | ayush.khare |
#44 | 2838343-44.patch | 49.35 KB | ayush.khare |
#40 | 2838343-40.patch | 54.98 KB | urvashi_vora |
#38 | 2838343-38.patch | 71.39 KB | andypost |
#35 | 2838343-35.patch | 54.76 KB | andypost |
Issue fork ds-2838343
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:
Comments
Comment #2
andypostAfter phpcbf
Comment #3
aspilicious CreditAttribution: aspilicious commentedComment #4
martin_qI'm reviewing this at MountainCamp 2017 and will gradually grow the revised patch; posting partial versions in case I run out of time.
Adding an empty doxygen block may pass the code sniffer but actual documentation would be more helpful.
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.
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.
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.
Comment #5
martin_qA 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 wholeif
statement should really be one line:This is long, though technically allowed - but perhaps an extra level of nested if statement would be useful?
I'll leave it at the single-line version for now, though.
Suggest actually putting something in the docblock:
Ensure that this object is not cached.
Comment #7
martin_qFixed 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:
Comment #10
aspilicious CreditAttribution: aspilicious commentedThnx for all your work, I reviewed the patch and committed it.
Maybe you can fix some off the remaining warnings. :)
Comment #11
andypostI'm sure at least
should be fixed
Comment #12
andypostwe mostly everywhere using "get"
Comment #13
andypostDrupal practice shows more, fixed most obvious
Remains are
Comment #14
andypostupdated IS
Comment #16
andypostproper patch
Comment #17
martin_qSome further issues addressed.
Comment #18
martin_qMeanwhile, 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.
Comment #19
martin_qThe 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.
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.
Comment #21
martin_qRerolled patch 19
Comment #23
martin_qRerolled 19/21 again.
Comment #24
martin_qUpdated issue summary with current status of patches 17, 18 and 23.
Comment #25
aspilicious CreditAttribution: aspilicious commentedOk in an effort to reduce the errors. New patch.
Comment #26
aspilicious CreditAttribution: aspilicious commented@martin_q I missed your patch. If this one gets in, could you reroll it on top of mine?
Comment #27
martin_qHi @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.
Comment #28
aspilicious CreditAttribution: aspilicious commentedComment #30
andypostCurrent state is https://www.drupal.org/pift-ci-job/642628 39 warnings
Comment #31
andypostHere'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
Comment #33
aspilicious CreditAttribution: aspilicious commentedWe can't change the interface (not even type hinting) as it is a BC break.
Comment #34
andypost@aspilicious There's no changes in interfaces, just fixes for documentation
new patch
- fix test failure
- clean-up inline type-hints
Comment #35
andypostA bit more clean-up
Comment #36
andypostI filed separate issue #2868000: Document DsFieldInterface & DsFieldTemplateInterface
Comment #37
aspilicious CreditAttribution: aspilicious commentedThis needs a reroll after all the recent changes.
Comment #38
andypostReroll, some parts are fixed + new ones
Comment #40
urvashi_vora CreditAttribution: urvashi_vora at Valuebound for Valuebound commentedPlease review the patch. Thanks
Comment #42
swentel CreditAttribution: swentel at eps & kaas for Dropsolid commentedComment #43
ayush.khare CreditAttribution: ayush.khare at OpenSense Labs for DrupalFit commentedComment #44
ayush.khare CreditAttribution: ayush.khare at OpenSense Labs for DrupalFit commentedAdded reroll diff for #40 and also removed some phpcs issues.
Comment #45
emarinho CreditAttribution: emarinho at CI&T commentedI'll review it!
Comment #46
emarinho CreditAttribution: emarinho at CI&T commentedI've applied patch #44, but there still some work to do.
Listed some of the PHPCS errors and warnings that I've found.
Comment #47
emarinho CreditAttribution: emarinho at CI&T commentedI'll work on this!
Comment #49
emarinho CreditAttribution: emarinho at CI&T commentedFixed some of the CS and Dependecy Injections, please kindly review it. There are also some CS to fix:
Comment #50
LeoAlcci CreditAttribution: LeoAlcci at CI&T commentedI'll review it!
Comment #51
LeoAlcci CreditAttribution: LeoAlcci at CI&T commentedThank 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 =).
Comment #52
christyanpaim CreditAttribution: christyanpaim at CI&T commentedComment #53
christyanpaim CreditAttribution: christyanpaim at CI&T commentedComment #54
adaucyjI'll review it.
Comment #55
adaucyjI'm working on it.
Comment #56
adaucyjNow remains the errors on test files. I'll keep working on it.
Comment #57
adaucyjAll PHPCS errors/warnings were cleaned up. The only ones left are regarding the issue #3223617 and will be fixed there.
Comment #59
swentel CreditAttribution: swentel at eps & kaas commentedThank you everyone, merged! Woohoo!
Comment #60
swentel CreditAttribution: swentel at eps & kaas commented