[Policy, no patch] Add special tag to identify issues ready for a high level only review

If there are reasonable comments & phpdocs to start with, us pedants can do follow-on issues to help improve the formatting after the fact.

Basic things should include:

• Two spaces rather than tabs,
• Brackets in the correct location,
• OOP conventions in place,
• Comments - they don't have to be perfect, just enough to explain the code.

Maybe a "coding standards" tag could be added to issues that need some brief follow-up work to do some minor tidy up work (if there isn't one already)?

Maybe a "coding standards" tag could be added to issues that need some brief follow-up work to do some minor tidy up work (if there isn't one already)?

I very much like that idea. When we introduced change notices on Drupal.org, I was very hesitant to the idea of leaving issues open post-commit until someone writes a change notice for the change. But, with exceptions, that seems to work very well. We could use the same process for code style with a "Needs code style clean-up" tag (I don't think re-using the existing "coding standards" tag would be a good idea, but that's obviously up to discussion) and instead of marking the issues critical on that (which we do for change notices) we could go to "major", which would still make sure that we don't introduce code debt. Then new functionality would not be blocked on nitpicky code-style reviews, but on the issue queue thresholds (of which those would then be a part). But contributors are already used to that by now.

Cool idea!

Since @Lars Toomre was the apparent cause of the creation this issue, let me quote from #1161486-59: Move IP blocking into its own module,

Reading through the patch, the following items should be addressed in a follow up issue to bring this new module up to D8 documentation/coding standards.

I am at a loss as to why there is a problem here or with Drupal's quality review process. Could @sun explain further please?

It's also well within our abilities to add coder review to the drupal.org testbot, however there is a dearth of people interested in learning/developing the testbot platform. As @jthorson mentioned at DrupalCon Munich, he says that they currently have about one-half a person actively working on testbot (neither him nor Jimmy Berry really have the time to seriously invest). I rarely get offended because testbot rejects my patch since it doesn't bring an attitude, and it can provide much more comprehensive code-style corrections with less effort than humans.

@sun With regard to your comment in #5: If I created a follow up issue (as suggested), would you (or other core participants) pay attention?

Based on prior experience, it seems that it is better to hi-jack critical/major patches to get things right? Am I wrong?

I agree balancing our bare code style reviews / code comment stuff with getting things done would be important. With 10 weeks left, we cannot really consume people doing docs cleanups instead of features built. Docs cleanups will be possible later. All the quality checks went in though because there is no explicit trust that anybody would come back and do the followups, and when other changes get in, it becomes really hard to roll back something. So its a multi-edged sword. Practically though I agree focusing on features instead of exact wording in docs or wraps in comment lines is beneficial.

@quicksketch:

I rarely get offended because testbot rejects my patch since it doesn't bring an attitude, and it can provide much more comprehensive code-style corrections with less effort than humans.

I would actually go as far as saying that if the testbot would give a report on the deviations from the coding standards, it would not only save a lot of manual work being done in the review process, it would also be educational for everyone. Optimal would be if the default visible report is as short as possible, but with an option to expand it to give more details as well as explanations about the correct way.

I also think that part of the reason you don't get offended is that its not someone opinion, more than indirect stemming from those involved in the defining of the coding standard. Especially as you reaction to an individual reporting it also is influenced by the degree of like/dislike/respect/etc you have to that person.

If this would be implemented in the testbot, maybe the step after that could be to also give it a feature where it can reformat the patch with corrections. Maybe even allowing me to select what corrections should be included in that process.

Rethink what level/granularity of code reviews is appropriate for major changes

Is it possible to figure out and separate changes for major on not-really-major? It's very hard...
otoh having mess of badly documented code in HEAD could lead to unpredictable results and also make a contribution to slow down.

I think then major changes should have much more better summaries first, to make more eyes to make review... but "sandboxed" approach with 1M patches makes it harder. Also change notices mostly contains only a small amount of information - we need to fix it!

I'd draw a distinction between substantive and non-substantive style issues. A missing docblock, or a docblock that is factually wrong, that's a serious issue and a patch/branch should not be committed until it is fixed. Whether or not there's a space between the @param lines and the @return line, or if the @see lines are before or after @param or if the first line of the docblock is 82 characters instead of 80? That doesn't actually impact anything other than our sense of style and pedantry.

I'm fine with holding up a patch, even a major one, on "the API docs are missing", because that's something that's harder to do later on after everyone involved has forgotten how it works. (That's why you want those docs to be good!) It's needlessly excessive to hold up a patch on "there's 3 spaces here instead of 2" or "the use statements at the top should be in this order instead", or other purely mechanical, non-substantive issues. The latter is something anyone can fix at any time, and we've got lots of time even after code freeze to tighten up one-space-vs-two questions.

As usual, Crell nails it: formatting issues are followup, meaningful comments are mandatory. Is that guideline enough? (note that phpstorm can generate doxygen for your functions/methods and missing class properties. useful. edit: of course, editing that is still necessary but saves a lot of typing)

@Crell I agree with your general thrust in #11.

How would you characterize missing @param and/or @return type hinting? As Drupal moves more toward OO code, I think that these are necessary for the initial commit. Do you agree?

I'd say @return is mandatory, because that's what IDEs would use. For @param, they should be there but the really important place is in the method signature itself, where the code would actually enforce it. The @param is slightly redundant in that case (although we should still be doing it). The important thing is that both @param and @return make it obvious what is expected in and out, so you never have to find yourself guessing "OK, this is supposed to be an array... and that helps me HOW?" (Which is the case in a few places in core but especially all over contrib.)

Larger projects should be developed in their own git repository rather than using the patch workflow. Working with patches is slow, and increases the barrier of fixing issues we run across (particularly with syntax-related ones like the fore-mentioned). I'm all for code quality, even when projects are pushed behind. But, if these projects are hosted on their own repository and issue workflows, then these projects are never stalled.

This is also the result of Drupal's continual code growth. Working on one monolithic project can be difficult as there are just a vast amount of tests, modules, themes and inter-dependencies to take into consideration. Organizing these into manageable sub-projects is one of the goals of the Framework Initiative. Encapsulation is a key missing point in Drupal, and something that would really help decouple all these systems. Decoupling our systems would improve not only code quality, but also speed up project development as there would be less things people have to worry about when submitting a patch.

Rob: I agree with everything you said, but I don't understand how it relates to this issue.

Also... I am not sure how to put my feelings into proper words for starting this issue. What about this: I love you, sun.

Nice sentiment. :)

We'll want to make sure to give Jennifer Hodgdon to chime in here since she's the primary one who developed the documentation gate (which I notice now doesn't even include coding standards, ha!). But for myself, speaking as someone who is extremely proud of the consistency in our code base and a big proponent of applying coding standards consistently everywhere, I can only give this proposal in #12 an emphatic +1.

It's become increasingly clear to me that extremely well-meaning people who care about consistency end up wreaking absolute havoc on core developer morale with "drive-by Dreditor," particularly when an enormous mountain is just about to be moved, architecturally-speaking. It's been particularly bad lately because we're all under a lot of pressure with feature freeze coming up. Even despite the morale issue, however, I feel we have a lot more infrastructure in place now (the "Novice" tag, Core Mentoring Hours, DrupalCS, etc.) than we did during Drupal 7's development cycle to ensure that this sort of consistency-ifizing doesn't fall through the cracks if we don't do it up front.

I do want to be really clear though that to me, this sentiment extends only to those things that could actually be classified as "Novice" follow-up tasks and/or automated by DrupalCS: indenting things consistently, removing trailing whitespace, putting proper newlines around @params, making sure function comments start with "Verbs" and not "Verb," etc. This does not mean leaving blank PHPDoc, missing api.php examples, sloppy comments about how a tricky bit of code works, etc. to follow-ups. IMO those things have to be in the initial patch, because only the people who wrote the patch will understand why they're there, and history shows (actually, even "present" shows... 4/16 critical tasks right now are outstanding change notices...), people who make these big patches are notoriously bad at coming back and cleaning up docs work after. They desire to instead move onto other big patches. So we need to take that into account, by making sure that any code that's committed is understandable by more than just the authors of it.

I believe though that this is more or less what #12 is saying, so +1.

Heh, I guess another way to write #12 is "stick to the documentation gate": http://drupal.org/core-gates#documentation. Those sum up the really important points to me:

* Issue summaries for unwieldy, sprawling, and/or difficult issues.
* Make sure when you add or change stuff, the docs get added or changed in parallel.
* Anytime you touch a module hook, add/update its api.php accordingly.
* Make sure when you add a new module you document to end users what the heck it's for (hook_help())
* When you break BC, leave a change notice.

All of those are good reasons to hold back a patch. Trailing whitespace and other things that could be script-fixed (or Novice-fixed, in absence of a sophisticated enough script) is not.

Between #11, #12, and #18/#19, it sounds like we are all saying the same thing. :-) Now we just need to get Jen in here to codify it.

Yay, consensus!

Oh, xjm and zendoodles would be other people I'd love to hear from before declaring consensus. They've done the most to rally newbies around fixing these things after the fact, and I'd want to make sure that if we go this way this doesn't create undue hardship on them.

Btw, jhodgdon is currently on vacation, should be back next week on Tuesday or so. (actually reading emails is important :P)

@timplunkett mentioned this issue to me because I was mentioned by name. So, replying.

Rerolling patches with small changes is an excellent opportunity for novices because:

• It teaches them the mechanics of using git and the issue queue.
• It familiarizes them with something in the codebase.
• It's a task that can be done quickly and with a high success rate.

These sorts of cleanups are their own task types for office hours, and they are populated almost exclusively as spin-offs from patch reviews I do. (When I got busy with MWDS and VDC, we ran out of them.) Having a tag that core developers apply to an issue would make it easier for mentors to find these cleanups so it's not just up to us to come across these sorts of patch cleanups when looking for tasks.

That said. I did not invent an issue tag for this like I did for a number of other tasks, because I don't like the precedent it sets. My concern is that if we go the direction of "oh that's just a code style issue; we can fix it after commit," there is even less incentive for developers to read 1354 and learn useful, relevant things about how to better document their code. (If a @param or a one-line summary for a class seems documentation for documentation's sake, that means it's likely not being used properly.)

I also, conceptually, am uncomfortable with the idea of making it a policy that we perpetually have newbies clean up things that "real" core developers can't be bothered to pay attention to. As a novice last winter, I deeply resented that I spent dozens, probably hundreds of volunteer hours trying to clean up core to its own standards and reviewing work that other, less invested novices were doing to the same end, only to elsewhere in the queue see patch after patch after patch going in despite violating the rules I'd managed to learn by simply reading the darn standard. I've gotten feedback from other novices who've participated in docs/and or coding standards cleanups issues, and they confided exactly the same thing.

Yes, the drive-by Dreditor is a bane on our issue queue (especially when people don't bother to do a real review of the code nor the docs, and especially when they don't even consolidate the points from their Dreditor review into something educational and instead leave a string of the same terse nitpicks in issue comments that span pages). Yes, it's tedious and disheartening and derails issues and puts people off core development. But so does treating novice developers like maids for our patches.

The real solution that @webchick convinced me of last winter is to get advisory reviews working on testbot. Yeah, maybe it'd be annoying for testbot to throw your patch back at you because you left some trailing whitespace or wrapped your code incorrectly. But you know what? You'd then configure your editor properly and not have the same mistake again. The problem is that no one has had time to actually bring it home. I tried. I failed.

I've no intention of arguing with anyone about any of this and I'll be unfollowing this once I post, but since I was mentioned here, I've replied. I'm burned out on these discussions and I'll just close with a plea that people who care about good documentation actually review the documentation, not just its mechanics, and take the time to give constructive reviews. And sorry for making a whiny and probably largely useless post, but I can't pretend that I like this idea.

Meh, okay, one more thing that's actually constructive, I promise. I think separating a "clean-up patch" from the original patch negates a lot of the benefits to novices I outlined above. They're no longer collaborating with experienced developers on a real, functional change and they're no longer being onramped into the middle of the core process. They're being tacked onto the end of it.

Hat tip to #24. Thanks @xjm.

Let me elaborate on an example. Based upon my limited experience, many core patches lack type hinting for the @param and @return directives.

Those patches go in as is, but when attempts to add type hinting, the documentation core committer assigns a lower priority to such patches because such follow-up patches are difficult and time consuming to review. On top of that, I have seen many instances where the person behind the primary patch had no input on the documentation follow-up patch whatsoever. I am left to wonder why one should open up documentation follow-up issues if both core submitters and core committers assign such a low/ignore priority.

Well, those are all certainly good points:

1) We do not want to get into a situation of creating two "tiers" of core developers, with the "lesser" tier being essentially janitors to clean up after the "greater" tier. We want novice and experienced contributors collaborating together, because that's how we grow new experienced contributors.

2) We also don't want core devs divorced from the responsibility of cleaning up their own work. History shows that there is typically a weeks- or months- long process to complete a "minor" follow-up task, if at all.

Just brainstormed about this with xjm in IRC for a half hour or so, ruling out various options. (For example, follow-up tasks are unlikely to be completed, but attaching a brand new patch to an existing, already-committed issue is likely to just confuse the crap out of people.)

How about this as an idea? A new issue queue workflow status "needs coding standards clean-up", which falls between "needs review" and "reviewed & tested by community."

Pros:

1) We make it clear during "needs review" that you are to review things of material importance to a patch: docs, naming, use of APIs, code flow, performance, test coverage, whether or not it actually works. :)

2) We don't force novice contributors to become "maids" of the issue queue, working in some other dark corner away from the "pros" who are too good to clean up their own messes.

3) We don't end up marking issues "fixed" that are not yet, and losing track of follow-up issues.

When an issue is marked "needs review" and not "needs coding standards clean-up" you could still find problems with the coding standards (after all, it's kind of hard not to notice them if you're reading a patch and very familiar with the standards). But your coding standards feedback during "needs review" stage would need to come in the form of an updated patch, and not a litany of things for the patch author to fix.

Once a patch gets to "needs coding standards review" status, then "Drive-by Dreditor" is appropriate, and we can put the novice brigade on pushing the patch through its final paces.

Thoughts?

When an issue is marked "needs review" and not "needs coding standards clean-up" you could still find problems with the coding standards (after all, it's kind of hard not to notice them if you're reading a patch and very familiar with the standards). But your coding standards feedback during "needs review" stage would need to come in the form of an updated patch, and not a litany of things for the patch author to fix.

+1.

I'd add that a great way to make that updated patch would be to do what chx does, and mentor someone new (edit: an interested novice, not the patch author) through making those changes in real time. It's a great way to get someone contributing. I mean, hey, it worked on me. :)

Edit 2: An updated patch with interdiff. :)

I agree completely that Novice != maid. The key issue, I think, is timing.

To take a recent example, Lars did a thorough read through and spacing nitpick of the routing patch. Just about everything he listed was correct, but it was a distraction because at that point the patch was still heavily in flux, and our focus was "why are there 200 failing tests? You make no sense, Drupal!" There were also non-small chunks of code whose whitespace formatting was totally wrong because it had been copied and pasted from Symfony, and I hadn't bothered to clean it up yet because there was a non-small chance I'd be deleting it again in a day. :-) So some way to flag "OK, now it's ready for nitpicking" on an issue would, I think, help quite a bit.

I wouldn't use a workflow state for it, because that implies that every issue needs to go through that as a separate state, and that's yet another layer of "needs review... anybody? Beuler?" that would only slow down many issues. If an issue is only 3 KB in size, there's no need to separate it. If it's 200 KB, diddling about 80 chars vs. 81 chars on a line while code is still being deleted and rewritten daily is harmful. Rather, I'd suggest a "needs standards review" tag, rather than workflow, with an understanding that for "small" patches it doesn't need to be a separate thing (for some highly squishy definition of small).

I would also still point out that there is a qualitative difference between "your method doesn't have a docblock and is named wrong" and "there should be a line break between @param and @return". I don't think there's a hard line we can draw about what is substantive and what is not, but we do need to understand, and educate people on, the difference.

Tag vs. workflow state I'm relatively ambivalent on, but I will say this in defense of workflow states:

1) If the code has already been through a coding standards review during the "needs review" process, it wouldn't need to go through this phase at all, and could just go straight to RTBC. They could always be kicked back down from there if something was missed. Similarly to how when I fix an issue that needs backporting, I just move it straight to patch (to be ported), I don't bother moving it to fixed first.

2) Unlike issue tags, they are easily discoverable (how would a new person reviewing an issue know that such a tag even exists?), and impossible to misspell.

Also, regarding "Your method doesn't have a docblock and is named wrong" is IMO covered by the documentation gate. We're not proposing dropping anything from that gate here. Simply moving the coding standards "gate" (which doesn't actually exist, but is nonetheless rigorously adhered to).

Re: #28:

We could also flip it a little from what @webchick describes so that we add a new state before NR, something like "needs architectural review." (Then we even can have a color for it; it can be orange.) Don't change what NR means, but NAR means "We are tearing this up and down debugging its test failures and so forth, so let it be please with the coding standards."

Edit: In some ways though I like webchick's state better than this suggestion, because it makes issues that want code style cleanup super findable for us for core mentoring and for new contributors looking for a place to jump in.

The key thing in either case is that it's not an extra review state to go through. You can go straight from either to RTBC if someone thinks it's RTBC, and you can go straight from NW to either. But the compromise is that we're not relegating following coding standards to a tag (since people have to remember to add tags); we still expect to follow them before RTBC. On the other hand, there's a clear way for the developers to say "keep down the noise until we've solved these architectural questions, please." If you want to clean up coding standards in the earlier review state, you do so with your own patch and interdiff, and you do not kick the patch to NW over such things while it's still under active development.

I think that we can and should automate coding standards reviews. drupalcs is pretty comprehensive - certainly enough to catch the most common coding standards errors.

I'm of the opinion that, during a test run, drupalcs should be run over HEAD, the patch applied, and then run drupalcs again. If there are more coding standards errors with that patch, then the testbot should mark the patch as failed. I really hate that coding standards are not really standards at this point -- only suggestions -- and it really sucks that patches from a new contributor are rejected on the basis of coding standards errors and gigantic mega-patches are not. Regardless of the size of the patch, standards are standards and I'm of the opinion that people contributing patches should take responsibility for making sure that their code conforms to those standards.

Why not just make sure you're following coding standards on the lines that you're changing, rather than making a mad scramble at the end to clean things up before it gets committed?

cweagans: I completely agree that automating is how we should ultimately fix this, and I think everyone else would agree. But we're unfortunately a ways off from DrupalCS + integrated testbot + no false-positives. See #1518116: [meta] Make Core pass Coder Review for a sobering glimpse at how frustrated people are with trying to make that work. That doesn't mean it's not important to work on, but realistically core-wide compliance + automated tools will be (and should be, honestly, since it's not the right focus at this point in the cycle) a post-feature freeze task.

So in any case, we need a solution that can get us through the next N months before automated checks are widely available. Such a proposal is in #26.

@webchick OK... I am now lost.

At what point should a key initiative patch go through a thorough documentation/coding review? At what point are we of 'the lower class' suppose to review the patches associated with key initiatives? Follow-up documentation issues are generally ignored and are left to languish forever before they even might be committed. Core contributors have moved on to their next pet 'issue'. For instance, at what point should type hinting be added to @param and @return directives?

If type hinting @param and @return directives were part of the documentation gate (which it's not, afaics? is there an issue you can point to where it was approved to add this to the documentation standards?) then this would happen during "needs review" stage, like all other items of material importance to the patch. The documentation gate specifies that In-code API documentation must be added or updated when surrounding code is added or updated, so this is already covered.

Other minor things, though, like "one newline instead of two" or "wrap at 80 chars" or "Comment starts with Verb instead of Verbs" would be done at the later "needs coding standards clean-up" phase, because those things are easily doable by novice contributors (and, eventually, a bot).

The proposal in #26 specifies that both of these things would need to happen in order for a patch to get marked RTBC, meaning that coding standards clean-up crew + patch authors are collaborating together on the ultimate fixing of an issue. So I'm not sure where you're getting this 'lower class' assertion, nor the assertion that we'd be spinning off ignored/languished sub-issues for this.

Oh, ok. Found it #711918: Documentation standard for @param and @return data types. And it's actually documented at http://drupal.org/node/1354#param-return-data-type but this is very confusing because the entire rest of that page just shows @param $foo.

Anyway, so given that type-hinting is part of the documentation gate, it would also happen during code review phase.

@webchick, I wasn't suggesting that we needed to make all of core pass right away, just that patches need to have a neutral or positive impact on the number of fails that we get (that is, the same number or less coding standards fails with the patch applied compared to without). This should be very very easy to do with our current testbot setup and would not involve a core-wide coding standards cleanup.

Uh, actually, after reading #711918: Documentation standard for @param and @return data types and http://drupal.org/node/1354#param-return-data-type I'm really confused. It doesn't sound like this is actually a requirement, more of a "judgment call" thing. So therefore it sounds like it wouldn't happen during code review except if someone was reviewing the documentation more holistically and found type-hinting helpful? Maybe? Uh. I have no idea. This issue is not the place to discuss it, either. Created #1792992: Are typed @param / @return part of the documentation gate?.

cweagans: The only way to determine that right now with the tools we have though is checking http://qa.drupal.org/8.x-status on day 1, re-checking it on day 2, determining if the number of "code review" fails went up, and if so, going back through git log to figure out what got committed between day 1 and day 2, and then re-opening issues for coding standards clean-up after the fact. Not fun.

To me, the only way automated coding standards reviews make sense is if testbot kicks back patches to "needs work" when they contain coding standards issues, so the problems are eliminated before patches even get to needs review, and so developers universally form good habits by default. But doing that does require a full-scale core code clean-up, plus elimination of false-positives in the code style checking tools, or else every single patch will fail. For reference, 8.x currently has 1,076 minor(s), 128 critical(s), and 633 normal(s) fails.

That said, please by all means talk to jthorson et al about how you can help make it happen, if you're genuinely interested in working on this. It would solve a ton of problems. But I can't say I'm interested in waiting for resolution of a long-standing feature request that has had some of our best minds working on it for over a year, rather than solving this soul-sucking contributor velocity issue right now by simply changing one setting in an admin form. We could always pull it back out if and when the bot does coding standards checks for us.

Lars: We're specifically trying to avoid a "lower-class reviewer" implication. Rather, we want to differentiate between "this is a good time to look at the code" and "this is a good time to look at the style/polish", since polishing before the code even works is a distraction and trying to revisit the architecture when the code is already in "finish the docblocks" mode just annoys people who have already poured a ton of work into an issue. If someone wants to focus on one or the other of those types of reviews, great.

That said, rereading sun's original post I have to ask if he was also referring to loosing up our pedantry level a bit in general, not just restructuring when it happens. Sun?

How about this as an idea? A new issue queue workflow status "needs coding standards clean-up", which falls between "needs review" and "reviewed & tested by community."

I think the key here is the "needs review" status. My hunch is that many in the community reads that as the patch is functionally ready and needs to be cleaned up before making it RTBC. Especially when testbot reports green for it.

To avoid having users nitpicking about following coding standards, maybe a better idea is something before the "needs review" stage. In publishing we have "draft" -> "review" -> "publish" as the most simple workflow stages. Most people understand that a draft is unfinished work in progress and avoid being too nitpicky.

Transformed to coding workflow it could be "patch draft" -> "needs patch review" -> "RTBC".

Edit: Had an old page up so didn't see @xjm's comment in #30, which seems to be similar to mine here.

#30 i would love to see a state before NR, only problem is that we need to communicate the fact that there's a new state and that it's an important one otherwise people will miss it (confession: I sometimes start looking at issues for the first time when they are in the NR state).

This probably also means that testbot should do it work in this new state?

Long term solution is to let the testbot handle the nit picking

I love #30. This process here is surprisingly positive, forward looking and altogether very awesome. We do not want to weaken Drupal's high quality standards, belittle the work who do coding standards review (gosh how do you guys catch a missing dot in a doxygen??) and that's exactly what #30 would bring.

Er, the goal is to get higher velocity. I don't think that adding a new, early state accomplishes that. I would prefer to use the coding standards tag to direct CS work. I'm also concerned that patches will decay will in the new state? Reroll fatigue is a problem too.

I really think worries about a "maid" class are overblown. Everyone contributes here in their own way. And we are all thankful to folks who fix up docs and to folks who add new features. Lets just keep celebrating the folks who clear that tag.

I definately agree that this is Critical, and has been for a long time. I'm one of those who hesitates to contribute because I fear the multiple rounds of nitpick assualt.

Is it feasible to ask that the nitpicks come in the form of patches, rather than Dreditor reviews? That would alleviate the tedium of one person having to constantly reroll patches, while also allowing the fabulous people who have this attention to detail to get more credit for their work.

@rbayliss:

I think this would be problematic in git workflows unless interdiffs are required. And big patches are likely to use git workflow.

That would be less problematic in a full git-based workflow, rather than our current hybrid, but that's unlikely to change any time soon either.

So baring automated code sniffing in a fully Git-pull based workflow, which we want but won't be here for a while, what's next? :-)

Although, perhaps issues that are using a Git-based workflow should have a tag to indicate such, meaning "please don't post direct patches, I can't work with those"? (Since I run several of those issues, I'm happy to start using such a tag if people feel it would be useful.)

I noticed this in my searching for new "coding standards" core issues, and see that someone above was asking if I could comment... Could someone edit the issue summary with any changes that are proposed to the current policies (i.e., the "gates") so that someone coming to this issue today could have a chance to see what the policy changes are proposed to be? I don't have time to read 47 long comments today... Thanks!

@Crell:

A tag would make sense anyway: we need a consistent way to warn committers that the changes should be merged and not committed.

When pedants sun and Crell agree that we're sometimes too pedantic, one should listen. That's what I think. :-) Well said, sun.

I agree with most of what sun said, up to the last paragraph. I do believe that some way to mark "this needs high level reviews, not pedantic line-by-line reviews yet" is valuable in itself, as is a way to mark "this is using a git merge workflow, please don't just post new patches, I can't use 'em". That does, however, seem like a somewhat separate issue from what sun is describing.

Of course, the challenge is opening up potential bikeshedding avenues, because we all know Drupalers love to debate minutia. One of the key reasons I pushed for our namespace "use"age standards to be extremely strict from the get-go was to eliminate any potential source of "well here maybe you should just use Foo\Bar, and then you can use all of those classes as Bar\Baz, but over here list them out, well maybe not enough, well..." My concern is that if, for instance, we say we can be fuzzy on 80-character first-line docs, then is "too long, even for a rough draft" 90 chars? 85? 100? The width of my screen, whatever that is?

I agree we should let ourselves loosen up a little to allow progress to happen. I am just concerned about unintended consequences and new opportunities to talk rather than code.

While I agree with the sentiment of "don't block big efforts with pedantic reviews", as expressed by sun in #49, it sounds like this issue is trying to advocate that something like this be added to the gates:

If this is a really big patch, we don't require the documentation and coding standards to be followed exactly at the first commit -- they can be cleaned up in follow-up issues.

Which of course would bring up the questions:
- What is "really big"?
- Which standards can be relaxed and by how much?
- What happens if the follow-up issue is never patched (or never filed in the first place)?

Given that it's vague, I personally don't think we can't really add a sentence like that to the gates or to our coding standards, can we? If you can come up with a better way to state this that doesn't introduce this type of vagueness, let's see it... Otherwise, maybe it's just a matter of convincing the most-current-branch maintainers, on a case-by-case basis, to commit big patches with what you've convinced them are small coding standards flaws that can (and will) be fixed in follow-ups?

We should really be distinguishing between documentation in the broad sense (including inline comments) and code formatting standards. As already stated above, a big patch lacking the former is a recipe for disaster, while the latter might be considered a nice to have in particular contexts. However I agree that the context should be better defined: maybe this would apply only for stuff subject to a deadline approaching in X days?

Created #1795750: Revise coding standards to use IETF RFC 2119 standards

A subset of the discussion in this issue that is narrower in scope and should be more actionable: #1809354: [Policy, no patch] Clarify point 2 of the documentation gate

The idea about introducing style checks by enforcing patches make a positive or neutral effect seems like a good idea that shouldn't be hard to implement. Basically same same approach for testing.

Not sure why this was a support request...

Presumably so it wouldn't hold up features.

So #1809354: [Policy, no patch] Clarify point 2 of the documentation gate is looking good I think. When we have consensus on that, here are the recommendations I'd give contributors:

• If the issue fails to comply with any of the core gates, including what is explicitly specified in #1809354: [Policy, no patch] Clarify point 2 of the documentation gate, it can be considered Needs work.
• Documentation or code style errors outside those specific points are not by themselves compelling reasons to mark an issue Needs work.
• Contributor reviews that specifically target documentation and code style are best offered by submitting a rerolled patch with an interdiff along with a brief explanation of the changes, rather than as a lengthy dreditor review.

+1 on #58 -- sounds very rational.

One of the goals defined here is:

Clarify Drupal [core] development processes.

I think #1583840: Define conventions around drupal core git interaction is really relevant for that goal, but the title of this issue does not really mention general development process, so I am a little confused.

It seems as though the "Core Gates" have set the standard for what constitutes patches that can be committed. For instance, in the area of documentation, they do not require that the docs be perfect; they do require that all functions and classes have documentation. It seems that this strikes a fair balance between having standards for what would be great, vs. letting progress happen?

If/when we ever get an automated tool in place to review and hopefully fix coding standards issues (such as whitespace, indenting, etc.) then we can codify what the real standards are and enforce them in a reasonable and equitable way. Meanwhile, if there are standards we should get rid of, file issues.

This issue is important and the discussion is necessary. It does not meet our definition of Critical:

Critical bugs either render a system unusable (not being able to create content or upgrade between versions, blocks not displaying, and the like), cause loss of data, or expose security vulnerabilities. These bugs are to be fixed immediately.

https://drupal.org/node/45111

As we march closer to a beta release, the Criticals in our queue indicate the issues that must be resolved in order to propose a release candidate. Smelly code that works still works. And we can continue to improve docs in minor releases.

I'm pushing this down to Major.

Just a note this is a support request, so it's not included in any blocking stuff.

The handbook says:

Support requests should never be marked "critical" or "major".

It's not really a support request.

It has been 7 years since there was discussion here and I see there is no proposed resolution in the issue summary.

I read the comments and understand that that is working to address the sometimes negative impact of coding standards reviews. But since this issue was created coding standard checks have become automated. And as the sniffs get enabled in core there are less reasons for these types of reviews.

Still, it is true that not all the rules for the Drupal coding standards are enabled. Issues still do get set back to Needs Work when they don't meet the Minimum requirements for in-code documentation. Is that still causing a problem for some?

I do think that in the 7 years since this issue became quiet that the community practice has improved in this area and that this can be closed.

I think the overall problem is still very much there - critical bugfixes often get stalled on something like documentation wording for the test coverage - I know this because I sometimes mark the issues needs work for that, obviously in the hope it's a quick fix and I can commit it when it's back at RTBC soon, but that's not always the case. Sometimes I try to fix things on commit, and sometimes reviewers fix things 'on-review' (i.e. upload a patch/commit to the MR with their remarks addressed), which is a good compromise between deliberately ignoring problems to get something committed vs. stalling an issue on something minor.

But sometimes I find a major bug that's about two years old that got marked work for a very minor issue, and then never got revived and just sat there, and it's very sad when that happens - especially if it's an issue affecting real sites - and then I think 'was the grammar of that code comment really worth the two year delay?'

There are these issues to remove boilerplate documentation that we don't really need any more:
#3238192: Allow omitting @var for strictly typed class properties
#2140961: Allow constructor methods to omit docblocks

There's also this issue to only add tests we actually need, and/or sometimes in follow-ups #2972776: [policy, no patch] Better scoping for bug fix test coverage.

These would help to remove the 'surface' of things that need review at least, but there are probably more, similar changes that might help too.

PHPStan, cspell, and also bugsmash and the needs review queue initiative are all definitely helping the overall situation, since at least you get much quicker feedback vs. waiting three months for a review then getting your issue marked needs work for a typo. PHPstan and cspell however are also reasons I wouldn't want to punt too much to follow-ups, since we already have massive number of coding quality issues via the process of fixing phpstan levels - we don't want every medium-priority issue to span 5 minor follow-ups.

@catch, Thanks. I really appreciate your ability to summarize the history and to provide examples.

The three issues linked are definitely part of improving the review process. And they are on my list to work on, if that helps.

For this issue, I have updated the Issue Summary with the suggestions from the comments. I also added one from a relevant Slack discussion that catch started in #needs-review-queue-initiative.

I suggest the next step is to get consensus on an Option.

I like option A - but alternatively we might be able to use this to solve two problems we have at the moment, the other one being a lot of poor re-rolls and contributions that only address these types of nitpicks and don't do anything to resolve open issues.

So perhaps we could keep it as needs review, and
* if you have nitpick standards reviews AND
* you don't intend to fix them yourself

Then you keep it as needs review, leave your review and add a tag such as Novice (or a new tag) for the new contributors to come along and solve those bits in a useful fashion rather than disrupting another issue.

But yeah, if that's not popular, option A sounds fine.

I'd like BOTH options A and B.

> A new status 'Needs architectural review' to be used before 'Needs review'

This would be REALLY useful. There's no point nitpicking details and polishing code based on nitpicks if the whole approach is wrong. And sometimes, it's enough to just write out an outline of how something would be coded.

And 'In progress' would be useful too. Especially now we have bots that set an issue to NW if the MR no longer applies, when all you want is some extra pairs of eyes on it.

fwiw I also like option A, although maybe we could go for a single 'In progress' status and use a 'Needs architectural review' tag?

The lifetime of an issue could then look like this:

Active -> In Progress -> Needs review -> Needs Work -> Needs Review -> RTBC - Fixed

If we'll go with follow-ups(backlog) for minor polishing it will bring 2 new problems
- size limits for new queue (new backlog)
- longer time to get stable state for new features

My example is help topics - years to get reviews

There are these issues to remove boilerplate documentation that we don't really need any more:

I also see the case for getters:

/**
 * Returns the foo service.
 *
 * @return FooInterface
 *   The foo service.
 */
public function getFoo(): FooInterface {
  ...
}

In this case I see the @return description redundant, having no value.

This was discussed in Slack by catch, smustgrave, lauriii, larowlan and nod_. Here is my summary of that discussion and the last few comments.

In Slack, the option A-D here were discussed as well as plenty of other solutions. That alone indicates that the options here are, at best, part of a number of improvements that could be made. It was also clear that the situation as expressed in the earlier comments in this issue (up to #66) are from when we didn't have automated coding standards checks. We do have them now. But the problem persists. It is less prevalent but it is still slowing progress on issues.

In regard, Options A-D here is what people preferred.

• Option A webchick, xjm, attiks, chx, smustgrave, catch, larowlan, joachim, FeyP, quietone
• Option B smustgrave, joachim
• Option C DameinMcKenna, moshe weitzman, tstoeckler, plach
• Option D lauriii

The 'Needs architectural review' status has the strongest support.

And these are the other suggested solutions.

• Do more self-RTBCs and keeping issues RTBC after posting changes
• Another options, 'E' Add status 'In progress' with tag 'Needs architectural review'
• An approval process (apparantly GitLab has this).
• More room for 'fixed on commit' from committers.
• Add a baseline for phpcs.
• Ignore non automated nitpicks, possible followup. These followup can be good for new contributors. However, there is concern about this due to the credit farming behaviors. Also, longer time to get stable state for new features (from #85).
• If you find nit-pick standard reviews, then update the patch yourself.
• Coding standards - remove boilerplate no longer needed

Let me know if I have made a mistake in the Summary.

edit: fix formatting

Adding credit for people in the Slack discussion. Let me know if I missed you.

I probably would have asked this during the work week, but I am away next week, so doing so now.

Is there agreement to proceed with Option A? I am adding that lauriii said in Slack that they are happy to support testing of the other options.

Is there anything blocking implementing Option A?

If we use a tag for option A, then it's easy. But to get a new status we'd need to open a project module or drupalorg (not sure which) issue.

Assuming that Option A is agreed to we also need text for the Issue tags -- special tags page. Here is a suggestion.

Needs architectural review

The issue is available for high level reviews only. It is not to be set to Needs Work for any minor or nit-pick change.

In Slack, dww pointed out that I was thinking that Option A was a tag, not a status. Comment #95 is incorrect. But some text will still be needed on Status.

I've been following this issue but didn't have a chance to chime in. I haven't read every comment, but I got through the extensive summary. It seems there are multiple, overlapping problems we're trying to solve. Thanks for everyone's efforts working on this important problem space.

I didn't see an option for what I believe is (part of) what we should do: a new 'Needs architectural review' issue tag (not status).

I updated the summary to use h4's with an id for each of the choices (way better for accessibility than strong), and now we can link to options as needed. 🤓

I also added Option E: A new tag 'Needs architectural review' and put myself down in support. 😉 Copying here as a comment:

Just like option A, but implemented as a new issue tag, usually for things still in 'Needs work' status.

Could also be applied to something in 'Needs review' if an issue is ready for both kinds of feedback.

Sometimes used along with the existing 'Needs XYZ review' tags we've already got (subsystem maintainer, framework manager, etc).

Not every issue needs this step, so instead of forcing everyone on every issue to think about a new status value, use a tag for the subset of issues that would benefit from this.

We could also adopt the new tag immediately by starting to use it, instead of having to wait for a drupal.org config change.

@dww, thanks for your input.

I suggest the we keep the Options closed now and put our effort into resolving this issue in the next few weeks.

Option C/E - add a tag.

Ideally, it should be automated.

- All commits are run through linters (PHPCS, PHPSTAN, Eslint, Stylelint etc.).
- Where possible, said linters would add a "fix" commit for anything they can correct.
- If there are still issues after the "fix" commit, a "needs formatting" (Needs architectural review) tag is applied.
- If there are no linting issues but the "needs formatting" tag exists, it is automatically removed.

The community encourges novices and workshops/camps to work on "needs formatting" tagged issues where possible. The goal being to make commits that will remove the "needs formatting" tag. If they can also contribute to the original issue in other meaningful ways, even better. This helps familiar them with best practices while focusing on smaller code chunks.

In a previous project in a Github workflow, I used php-cs-fixer-ga to scan commits & PRs, and git-auto-commit Action to commit any changes php-cs-fixer-ga created. That automated the first 2 steps above.

IMHO, another state in the progress chain would only slow the release of patches.

Updating the Option support in the IS, based on Slack discussion. https://drupal.slack.com/archives/C1BMUQ9U6/p1682043836509079

Option A webchick, xjm, attiks, chx, smustgrave, catch, joachim, FeyP, quietone
Option B smustgrave, joachim
Option C DameinMcKenna, moshe weitzman, tstoeckler, plach
Option D lauriii
Option E larowlan, FeyP, dww

I think given option E only requires a new tag rather than a status we could go ahead with that and combine the votes for A and E.

@catch, that is certainly a reasonable suggestion.

How about a trial period using the tag? We can start using it now and then in, say a month, evaluate the results.

So, are there any objections to creating a new tag, 'Needs architectural review', and using it for a trail period of one month?

Created #3358079: Add a new status 'Needs architectural review' for core issues to learn how about the time required to implement a new status for core issues,

dww responded in the issue created above explaining all the work required to add a new status. The conclusion there is that it is very unlikely a new status will be added. It would require code changes as well as doc change. And give that moving to GitLab is the priority there is little interest in doing the work.

So, that leaves with setting up a new special tag. We can start using a tag right now, but it would help to document its use.

Adding tag

I have implemented the tag and started to add credit.

More work on assigning credit.

Updating the title to reflect what the discussion has been focused on. I am taking the liberty of adding credit for myself for the work in updating the issue summary, adding the tag etc.

I think we have reached a conclusion on this issue so I am setting to RTBC.

For further improvements to the review process see the issues at #3351644: [meta] Improve issue management before making a new issue.

This is great, let's try to actually use the new tag!

Adding parent to help find related issues