The docblocks and coding standards in file core/modules/views/src/Tests/ViewTestBase.php needs some checking. The following errors were found.
------------------------------------------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
------------------------------------------------------------------------------------------------------------------------------------
35 | ERROR | [x] Missing function doc comment
83 | ERROR | [x] Type hint "array" missing for $result_set
98 | ERROR | [x] Parameter comment must end with a full stop
99 | ERROR | [x] Doc comment for parameter $label. does not match actual variable name $message
126 | ERROR | [x] Concat operator must be surrounded by a single space
------------------------------------------------------------------------------------------------------------------------------------
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | core-fix_doc_block_for_viewtestbase-2633682-14-D8.patch | 2.04 KB | anish.a |
Comments
Comment #2
johnrosswvsu commentedI am attaching the patch file that addresses these issues.
Comment #3
jmarkel commentedWhich version of Coder did you use? I suspect it's an older one, because of the line 83 array hint error - that error has been removed from the Drupal sniffs because array type hinting is not, as of yet, a Drupal standard, however good an idea it is to use it (and I'm not suggesting you change that one back :-) )
The current version of coder is 8.x-2.5.
There are a couple of codesniffer warnings that should probably also be mitigated:
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
52 | WARNING | Line exceeds 80 characters; contains 81 characters
112 | WARNING | Only string literals should be passed to t() where possible
----------------------------------------------------------------------
The second warning, about having only string literals passed to t(), is a little funky - here is its use, in context:
The variable being passed to the t() function is the $message parameter from helperButtonHasLabel(). $message is an optional parameter with a default set in the method's parameter list. As far as I can determine, helperButtonHasLabel() is never called with $message in the passed parameters - i.e. every existing call to helperButtonHasLabel() uses the default message text. That suggests to me that it would be better to remove $message from the method parameters and set the message, as text, directly in the call to t(). I'm not sure if this is quite in scope for the issue at hand, so I'd be interested to hear other opinions.
Comment #4
jhodgdon@jmarkel - I do not know why you unassigned the original person; it is VASTLY preferable if one person keeps working on an issue until it is done. Also updating status.
But this is not only documentation because of the type hint being added, so moving to Views module.
Comment #5
jmarkel commented@jhodgdon - My understanding was that, once a patch was complete and submitted for review, the issue should be unassigned. Guess I was wrong, and will not do that again.
Comment #6
johnrosswvsu commentedThanks for those catches:
I adjusted the line for maximum of 80 characters.
As for second issue I have made a change that might need extra checking. Since the standard only mentions "where possible".
The protected function in question are used in the following:
./core/modules/views/src/Tests/Plugin/ExposedFormTest.php:58: $this->helperButtonHasLabel('edit-submit-test-exposed-form-buttons', t('Apply'));
./core/modules/views/src/Tests/Plugin/ExposedFormTest.php:71: $this->helperButtonHasLabel('edit-submit-test-exposed-form-buttons', $expected_label);
./core/modules/views/src/Tests/Plugin/ExposedFormTest.php:84: $this->helperButtonHasLabel('edit-submit-test-exposed-form-buttons', t('Apply'));
./core/modules/views/src/Tests/Plugin/ExposedFormTest.php:127: $this->helperButtonHasLabel('edit-reset', $expected_label);
./core/modules/views/src/Tests/Plugin/ExposedFormTest.php:193: $this->helperButtonHasLabel('edit-submit-test-exposed-form-buttons', t('Apply'));
./core/modules/views/src/Tests/ViewTestBase.php:112: protected function helperButtonHasLabel($id, $expected_label, $message = 'Label has the expected value: %label.') {
./core/modules/views_ui/src/Tests/ExposedFormUITest.php:46: $this->helperButtonHasLabel('edit-options-expose-button-button', t('Expose filter'));
./core/modules/views_ui/src/Tests/ExposedFormUITest.php:58: $this->helperButtonHasLabel('edit-options-expose-button-button', t('Hide filter'));
./core/modules/views_ui/src/Tests/ExposedFormUITest.php:60: $this->helperButtonHasLabel('edit-options-group-button-button', t('Grouped filters'));
./core/modules/views_ui/src/Tests/ExposedFormUITest.php:81: $this->helperButtonHasLabel('edit-options-expose-button-button', t('Expose sort'));
./core/modules/views_ui/src/Tests/ExposedFormUITest.php:97: $this->helperButtonHasLabel('edit-options-group-info-add-group', t('Add another item'));
./core/modules/views_ui/src/Tests/ExposedFormUITest.php:154: $this->helperButtonHasLabel('edit-options-expose-button-button', t('Hide sort'));
Please check.
Thanks.
Comment #8
johnrosswvsu commentedGot to canel due to wrong patch file naming convention.
Comment #9
johnrosswvsu commentedHere is the correct one.
Comment #12
marvil07 commentedCode looks OK, but the patch no longer applies, hence marking it as NW.
Comment #13
anish.a commentedComment #14
anish.a commentedReroll from scratch.
Comment #15
dawehnerThank you for your work on cleaning up Drupal core's code style!
In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. We should also separate fixes where we need to write new documentation from fixes where we need to correct existing standards. This all should be done as part of #2571965: [meta] Fix PHP coding standards in core, stage 1. A good place to start is the child issues of #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard.
For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.
Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.
I'm going to close this issue, but please get involved in the child issues above.