Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|
Comments
Comment #1
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedWrong issue
Comment #2
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedComment #3
xjmSee also:
Comment #4
lotyrin CreditAttribution: lotyrin commentedRemaining complaints from coder:
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.
Comment #5
lotyrin CreditAttribution: lotyrin commentedForgot: 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).
Comment #6
lotyrin CreditAttribution: lotyrin commentedComment #7
lotyrin CreditAttribution: lotyrin commentedMore fixes thanks to drupalcs.
Comment #8
xjmCan you create a git diff patch for easier review? Thanks!
Edit: Also, 'ware the trailing whitespace. (Shows up in a few rewrapped comments.)
Comment #9
xjmIn addition to what I've suggested in #8, let's update the issue here with:
In each case note any failures that were not corrected, and why.
Comment #10
lotyrin CreditAttribution: lotyrin commentedReformatted patch, with the two instances of trailing whitespace fixed. Guess I didn't re-test after editing that file.
Comment #11
lotyrin CreditAttribution: lotyrin commentedUnassigning 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.
Comment #12
TravisCarden CreditAttribution: TravisCarden commented#10: coder-includes-ac.patch queued for re-testing.
Comment #14
cosmicdreams CreditAttribution: cosmicdreams commentedThis sounds fun, and this issue is already started for me. So I'll try tackling it this week.
Comment #15
TravisCarden CreditAttribution: TravisCarden commentedPostponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!
Comment #16
sphism CreditAttribution: sphism commentedWe have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details
Comment #16.0
sphism CreditAttribution: sphism commentedUpdate summary.
Comment #17
donquixote CreditAttribution: donquixote commentedSome piecemeal work here, maybe it is useful: #2210565: Docblock and code style improvements in core/includes/bootstrap.inc
Comment #18
donquixote CreditAttribution: donquixote commentedI 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)
Comment #20
donquixote CreditAttribution: donquixote commented@cam8001:
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.
Comment #21
donquixote CreditAttribution: donquixote commentedAll the patch rerolling :( I want pull requests!
Comment #22
TravisCarden CreditAttribution: TravisCarden commentedThanks, @donquixote. A few things here:
Please address these issues so someone can productively perform a closer code review. Thanks!
Comment #23
donquixote CreditAttribution: donquixote commentedi 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 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.
Comment #24
TravisCarden CreditAttribution: TravisCarden commentedWell, 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.
Comment #25
donquixote CreditAttribution: donquixote commentedThe 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)
Comment #26
xjmThanks 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.
Comment #27
xjmComment #28
pfrenssenClosing 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.