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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NROTC_Webmaster’s picture

Status: Active » Postponed

Wrong issue

NROTC_Webmaster’s picture

Status: Postponed » Active
lotyrin’s picture

Status: Active » Needs work
FileSize
4.31 KB

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.

lotyrin’s picture

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).

lotyrin’s picture

Assigned: Unassigned » lotyrin
lotyrin’s picture

FileSize
42.7 KB

More fixes thanks to drupalcs.

xjm’s picture

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

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

xjm’s picture

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.

lotyrin’s picture

FileSize
38.92 KB

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

lotyrin’s picture

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.

TravisCarden’s picture

#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.

cosmicdreams’s picture

Assigned: Unassigned » cosmicdreams

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

TravisCarden’s picture

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!

sphism’s picture

Status: Postponed » Active

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

sphism’s picture

Issue summary: View changes

Update summary.

donquixote’s picture

Issue summary: View changes
donquixote’s picture

Status: Active » Needs review
FileSize
16.99 KB

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.

donquixote’s picture

@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.

donquixote’s picture

Status: Needs work » Needs review
FileSize
17.91 KB

All the patch rerolling :( I want pull requests!

TravisCarden’s picture

Status: Needs review » Needs work
FileSize
4.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, fix Drupal.Commenting.FunctionComment.Missing*, 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!

donquixote’s picture

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.

TravisCarden’s picture

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.

donquixote’s picture

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)

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

Thanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.

xjm’s picture

Assigned: cosmicdreams » Unassigned
Priority: Normal » Minor
Issue tags: -Novice
pfrenssen’s picture

Status: Postponed » Closed (duplicate)

Closing in favor of #2571965: [meta] Fix PHP coding standards in core. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.