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.
Problem/Motivation
names of some variables in DI are wrong
docblocks were wrong also referring the wrong service tags or names
#2212411: Code style cleanup of language system did something similar.
Proposed resolution
Remaining tasks
- (novice) git instructions for creating patch | Contributor task documentation for creating a patch
- Review patch to check it fixes the issue, the change is properly documented and for coding standards. Make sure patch stays within scope of just this issue. | Contributor task documentaiton for reviewing patch
User interface changes
No.
API changes
No.
Comment | File | Size | Author |
---|---|---|---|
#20 | interdiff.txt | 797 bytes | Mile23 |
#20 | 2237101_20.patch | 4.39 KB | Mile23 |
#18 | 2237101_18.patch | 4.11 KB | Mile23 |
#17 | 2237101_17.patch | 4.47 KB | Mile23 |
#15 | interdifftxt.png | 132.88 KB | YesCT |
Comments
Comment #1
cordoval CreditAttribution: cordoval commentedComment #2
tim.plunkettBased on the OP and the tags, this sounds like a cleanup task
Comment #3
YesCT CreditAttribution: YesCT commentedwe dont actually make the code consistant... we make it match standards. :) there is a little difference. For example, if a long time ago, we didn't use newlines to separate code, but now we do. and we are fixing a bug in a file, then in our fix, we use the newlines according to the standard, and do not make it consistant with the rest of the file (which might not be standard).
adding documentation since this will touch/clean up documentation.
Also, if we separate out the documentation cleanups form any code at all, it might be more likely to get in, and we can do the code changes in a separate issue.
But we can post your whole patch here at least and get some feedback on how to do it.
--
using the issue summary template, and a few of the contributor links from (https://docs.google.com/document/d/1xnL18eC2Cwo2YnB0PwmBnn86Eo8aF9CF4qDL...)
--
I'm not sure this is major.
https://drupal.org/node/314328 might help decide.
and I think it's a task.
maybe this is base system?
Comment #4
YesCT CreditAttribution: YesCT commentedoh, forgot to link to similar issue. done that now.
Comment #5
cordoval CreditAttribution: cordoval commentedComment #6
tim.plunkettThanks for the patch! A lot of these cleanups are great, especially the ones where we clearly copied the variable names.
It's in our coding standard to leave a blank line after each method, including at the end of the class (here and throughout the patch)
https://drupal.org/node/608152#indenting
Unfortunately we can't add @inheritdoc like this, it either has to stand alone or not be there at all
https://drupal.org/node/1354#inheritdoc
Why remove these lines?
This should be wrapped to 80 char and end with a period
Generally leaving a blank line after these early-exit checks is better for readability
Comment #7
MixologicThis should probably be just "Contains", not "Contains of"
Comment #8
YesCT CreditAttribution: YesCT commented#6.2 yeah.
some people want to change that: #1994890: Allow {@inheritdoc} and additional documentation.
but for now, it is all or nothing.
[edit: tim had a good idea in irc. just writing it here so it doesn't get lost. "consider moving them to inline docs in the methods themselves" So, use inheritdoc, and instead of just deleting the docs that are adding information, also move that info into the methods as inline comments. :) ]
#6.4 yeah.
Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions (noted in the Tag Reference below).
https://drupal.org/node/1354#drupal
#7 yeah.
https://drupal.org/node/1354#file
next steps,
another patch.
also include an interdiff please. the interdiff instructions might help:
https://drupal.org/documentation/git/interdiff
oh, and when you upload next patch, change the issue status to needs review to send it to the bot, and signal people following this that you are looking for feedback.
Comment #9
cordoval CreditAttribution: cordoval commentedComment #11
cordoval CreditAttribution: cordoval commentedComment #12
cordoval CreditAttribution: cordoval commentedComment #13
cordoval CreditAttribution: cordoval commentedComment #14
cordoval CreditAttribution: cordoval commentedComment #15
YesCT CreditAttribution: YesCT commented@cordoval thanks for the interdiff posted in comment #9.
to keep the text bot from testing interdiffs, name them with a .txt ending (the testbot tests anything ending in .patch)
(by definition interdiffs will not apply to head, they should apply to head with the previous patch applied. :) )
in the interdiff doc: https://drupal.org/documentation/git/interdiff it is easy to miss that it should be .txt
You can upload more than one file at a time, usually we upload the interdiff and patch at the same time.
-------
So I think the file in #9 was the interdiff between the patch in #5 and the patch in #11. :)
-------
let's leave the instructions for how to make a patch and how to do a review in the issue summary until this is committed. :) We will probably need to do those things a few times more yet.
Comment #16
dawehnerNote: Most of the changes here are deprecated by #2213319: Create a single Container CompilerPass to collect + add handlers to consumer service definitions
Afaik we do use a leading slash here.
Comment #17
Mile23Here's a straight reroll.
Comment #18
Mile23Another straight reroll. :-)
This patch is getting smaller and smaller.
Comment #19
dawehnerCan we move down the documentation into the function body at least?
Comment #20
Mile23Added the documentation inside the method body.
Comment #21
dawehnerThose are all fair small cleanups! Thanks a ton
Comment #23
Wim LeersComment #24
catchCommitted/pushed to 8.1.x and 8.2.x thanks!
Comment #26
drummCorrecting issue status, see #2698635: Issue statuses changing by themselves?.