Part of meta-issue #1518116: [meta] Make Core pass Coder Review

These files currently fail the tests on QA .

I'm using my coder 8.x sandbox.

I'm deliberately ignoring any issues in docstrings, minor issues about 'drupal_*' string functions as well as the following:

core/includes/bootstrap.inc:
+1356: [normal] global variables should start with a single underscore followed by the module and another underscore
+2634: [critical] the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead
+2637: [critical] the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead
core/includes/common.inc:
+1968: [normal] global variables should start with a single underscore followed by the module and another underscore
+2579: [normal] global variables should start with a single underscore followed by the module and another underscore
+7454: [critical] Potential problem: trigger_error() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
/var/www/drupal/core/includes/config.inc:
+17: [normal] global variables should start with a single underscore followed by the module and another underscore
+94: [normal] global variables should start with a single underscore followed by the module and another underscore

Comments

Status:Active» Postponed

Wrong issue

Status:Postponed» Active

Status:Active» Needs work
StatusFileSize
new4.31 KB
PASSED: [[SimpleTest]]: [MySQL] 35,065 pass(es).
[ View ]

Remaining complaints from coder:

core/includes/config.inc:
+17: [normal] global variables should start with a single underscore followed by the module and another underscore
+93: [normal] global variables should start with a single underscore followed by the module and another underscore
core/includes/common.inc:
+1959: [normal] global variables should start with a single underscore followed by the module and another underscore
+2569: [normal] global variables should start with a single underscore followed by the module and another underscore
+7358: [critical] Potential problem: trigger_error() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
core/includes/bootstrap.inc:
+1356: [normal] global variables should start with a single underscore followed by the module and another underscore
+2630: [critical] the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead
+2633: [critical] the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead

The globals complained about here are used throughout core, so I don't think they fall under this rule.

I'm not 100% sure about the use of REQUEST_URI in bootstrap.inc (specifically request_path()) but it seems to be intended based on comments.

I'm fairly certain the warning about filtered text in common.inc (specfically debug()) is moot, as the caller of debug() should be responsible for filtering.

Status:Needs work» Needs review

Forgot: There's a docstring change that should happen that I avoided.

config_xml_to_array() is documented as having a $xmlObject parameter when in reality it's named $data (for good reason, because it's a string of XML, not a DOM object as the other name might imply).

Assigned:Unassigned» lotyrin

StatusFileSize
new42.7 KB
PASSED: [[SimpleTest]]: [MySQL] 35,071 pass(es).
[ View ]

More fixes thanks to drupalcs.

Can you create a git diff patch for easier review? Thanks!

Edit: Also, 'ware the trailing whitespace. (Shows up in a few rewrapped comments.)

In addition to what I've suggested in #8, let's update the issue here with:

  • Whether this directory either passes or fails on the Drupal 8 branch test code review tab. If it fails, also note whether the failures are itemized on that page.
  • Whether the directory has been tested with Drupal Code Sniffer with the patch applied, whether it passes or fails, and what settings were used.
  • Whether the directory has been tested with the D8 sandbox of Coder with the patch applied, whether it passes or fails, and what settings were used.

In each case note any failures that were not corrected, and why.

StatusFileSize
new38.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch coder-includes-ac.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reformatted patch, with the two instances of trailing whitespace fixed. Guess I didn't re-test after editing that file.

Assigned:lotyrin» Unassigned

Unassigning because I'm not sure I'll have time for these issues and I want to make sure people know they can jump in without stepping on my toes.

#10: coder-includes-ac.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice, +coding standards, +coder-fixes-2012

The last submitted patch, coder-includes-ac.patch, failed testing.

Assigned:Unassigned» cosmicdreams

This sounds fun, and this issue is already started for me. So I'll try tackling it this week.

Status:Needs work» Postponed

Postponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!

Status:Postponed» Active

We have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details

Issue summary:View changes

Update summary.

Issue summary:View changes

Status:Active» Needs review
StatusFileSize
new16.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch D8-2210565-1-bootstrap-inc-code-style_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I just tried to reroll (= apply + rebase) the patch from #10.
It shows tons of conflicts, because the files have changed quite a lot.

Imo, the easier approach is to start fresh, possibly with the patch from #2210565-5: Docblock and code style improvements in core/includes/bootstrap.inc.

Imo we can do this piecemeal. One file or one bunch of improvements at a time, so we don't have to reroll that often.
I am uploading the same patch for review again.
(There was already a review by cam8001 in #2210565-5: Docblock and code style improvements in core/includes/bootstrap.inc)

Status:Needs review» Needs work

The last submitted patch, 18: D8-2210565-1-bootstrap-inc-code-style.patch, failed testing.

@cam8001:

Shouldn't this parameter be documented?

For a start I added type hints to all parameters and return values I could find, but I did not change or add any text.
The former is straightforward, but the latter requires that I actually understand what this code intends to do - which is a lot more work :)

We can discuss text improvements as well, but for my taste we could commit the straightforward stuff first and then continue from there.

Status:Needs work» Needs review
StatusFileSize
new17.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,064 pass(es).
[ View ]

All the patch rerolling :( I want pull requests!

Status:Needs review» Needs work
StatusFileSize
new4.85 KB

Thanks, @donquixote. A few things here:

  • This patch changes only bootstrap.inc, but there are issues in ajax.inc and batch.inc which are in scope for this issue as well.
  • Adding doxygen type hinting has been descoped from this initiative and moved to #1800046: [META] Add missing type hinting to core docblocks, so those changes need to be removed from the patch. Please remember to follow steps 6 and 7 in the meta issue.
  • Make sure that the patch addresses all issues reported by Sniffer when run as described above. Attached is a report of those issues as of now.
  • Please don't do anything in the patch but fix coding standards issues.

Please address these issues so someone can productively perform a closer code review. Thanks!

i think the docblock fixes are actually the more important part of the patch.
Just found this issue: #1326456: Add missing @param in includes A-C, plus other corrections to some docblocks
It was closed, but I am going to reopen.

This patch changes only bootstrap.inc, but there are issues in ajax.inc and batch.inc which are in scope for this issue as well.

This might be one reason why these issues stay open forever. Instead of happily taking piecemeal improvements, we wait until someone comes along with the one golden patch which does it all, and then people lose their motivation.
But, whatever, I am not giving up yet.

This might be one reason why these issues stay open forever. Instead of happily taking piecemeal improvements, we wait until someone comes along with the one golden patch which does it all, and then people lose their motivation.

Well, this issue is specifically scoped to include all "files starting with A-C", so for the issue to be considered fixed as it's currently defined it needs to include all those files. If you want to attack it in smaller chunks, that may be fine if you want to create new issues with smaller scopes. Since the core committers originally defined the granularity of the tickets, though, you might want to check with them first in the meta issue.

The idea was to commit a chunk and then leave the issue open, so the next person has less work to do.

My scenario is, I was angry at bootstrap.inc, and out of this righteous anger I did some cleanup work that I want to share. But the righteous anger might not be enough to clean up the other files. So this way we may lose valuable piecemeal contributions from people with a (medium) motivation.

(In my case, I might actually go the full way and do all 3 files)