We need a process for updating Drupal coding standards changes and clearly communicating them to the community. These issues are piling up in the TWG queue and we need a process to manage their resolution and catch up on the list. After some community discussion, the technical working group is proposing the workflow spelled out below. We think it will be very much in line with the community vision based on the discussion and general consensus so far.

Proposed Workflow

A new issue queue will be created for the official Drupal Coding standards work (https://drupal.org/project/coding_standards). The maintainers of this queue will include but not be limited to the Technical Working Group members, all documentation maintainers listed in maintainers.txt, and additional maintainers can be appointed by the Technical Working Group. To avoid confusion, this project will function as any other Drupal.org project where community input is solicited but the maintainers will be the "committers" for the coding standards project.

The process below is documented on the https://www.drupal.org/project/coding_standards project page.

The Process for Changing Coding Standards

  1. Create an issue in the coding_standards project queue.
  2. At least two additional active community members (contrib module maintainers, active patch contributors, etc) need to agree to sponsor the change in the form of a comment on the issue.
  3. After an initial round of community discussion, the issue can be tagged "needs announcement for final discussion" and it becomes eligible for coding standards maintainer consideration.
  4. The coding standards committee use their discretion to decide which and how many of the "needs announcement for final discussion" issues to announce and tag with "final discussion".
  5. An announcement is made of the coding standards changes under consideration with an consideration date (e.g. 14 days). If there are concerns or conversation is on going, the issue time box may be extended.
    • The announcement will be made via TWG Twitter account, groups.drupal.org/governance, groups.drupal.org/core, and "this week in drupal core"
    • Issue is considered with the following guidelines:
      1. If a clear majority of commenters disagree with a policy change it is dismissed
      2. If no clear majority exists but core and contrib maintainers have advocated for the change, the issue will be automatically escalated to a private decision by the coding standards maintainers
      3. All changes that can be tested with coder/phpcs are advised to come with a patch on the the coding_standards project issue (to avoid a flood of patches for not-yet-ratified standards to the coder queue)
  6. The next meeting of the coding standards maintainers will discuss the issues and tag appropriate issues with "approved" and “needs documentation updates”, mark issue status as “needs work”, or close the issue with “won’t fix”.
  7. If the issue affects core it will be moved to the core queue and marked RTBC for core committer signoff after which the issues should be left in RTBC, tagged with “Core Committer Approved” and moved back to the coding standards project queue.
  8. Drupal.org coding standards pages are updated for approved proposals. Issues are marked "fixed" and “needs documentation updates” tag removed.
  9. An announcement of all active discussions for the period will be made providing links to updated coding standards docs where appropriate.

Comments

jhodgdon’s picture

That process is essentially the same as the "process" that has been in place for many years -- the only difference is that the issues used to be in the Drupal Core issue queue and tagged "coding standards".

The problem was that that process did not work:
- There was never enough discussion on the issues
- There was not a clear criterion for "enough discussion" or "who needed to concur" so that issues could be marked RTBC. Some of the issues I moved over here have been open for years.
- There was often not complete agreement. How do you decide in those cases?
- When decisions were made, even with the agreement of core committers, others said later "I had no idea this discussion was going on, this was a terrible idea!".

So I do not think this is a good process. There needs to be:
- more visibility of discussions (a process for announcing them)
- a defined period or number of comments or something (??) for cutting off discussion
- clearer criteria for when a standards issue can be marked RTBC - does everyone have to agree? only a majority? the core committers? what about contrib maintainers, who are also affected?
- a process for what to do if people don't agree - escalation?

tizzo’s picture

Issue tags: +Coding standards

Great points. So based on that feedback, maybe it would make more sense to have a process like the following (still just brainstorming here):

  1. Create an issue in the TWG queue
  2. The issue is added to the list of changes under consideration in a given batch (issues open before say a week prior to the TWG monthly meeting)
  3. An announcement is made of the coding standards changes under consideration with an end date (say 60 days?)
    • If a clear majority of commenters disagree with a policy change it is dismissed
    • At least one core committer needs to agree to the change
    • At least one contrib module maintain needs to agree to the change
    • If no clear majority exists but core and contrib maintainers have advocated for the change, the issue will be automatically escalated to a private decision by the Techincal Working Group
  4. The next monthly meeting of the TWG meeting the issue will be evaluated using the above criteria
  5. An annoucement of all changes for that session will be made

What I'm optimizing for: meeting needs of core and contrib, timeliness, community driven process, clear anouncements, preventing deadlock, ensuring TWG can keep up without taking too much control.

[Edited to fix HTML errors]

dawehner’s picture

I can't agree more. The process which got introduced wasn't tested / and just got introduced, which lead to a total stop of existing discussions.

If we introduce a new process, we need to test the process, much like we don't just commit patches but rather write a test / manually test it.

To get me clear, I don't say working groups is in general a bad bit for such generic discussions, but it has to be introduced with the community not against it.

tizzo’s picture

dawehner: you could not agree more with #1 or #2? What do you suggest as a next step for this group? What I'm trying to accomplish is moving TWG to fulfill it's charter and for it to stop being a place where coding standards issues go to die. ;-)

Are you suggesting these issues should be moved elsewhere? Or do you have a suggestion for the process?

jhodgdon’s picture

Just to be clear: The old "process", which was never put in writing, was essentially: "Make an issue in core, tag it 'coding standards', discuss for an unspecified period while mostly no one bothers to contribute to the discussion, someone (not clear who) finally decides, update the standards page, no one notices that either, people gripe when they find out". When the TWG was established with something in their charter about overseeing coding standards, I moved a bunch of old stale issues that had not gotten to the "someone finally decides" over to this issue queue, where they've continued to be in the "mostly no one bothers to contribute to the discussion" state.

Establishing an actual, written process, to replace this, would be a Good Thing.

I like the idea in #2 much better than the original post on this thread, because the original post is essentially the old "process" with one change, that the discussions are in the TWG issue queue instead of Core. That process was broken, in my opinion, in that decisions were not made after years in many cases. The proposal in #2 seems more definite.

But yes, we need to have a broader discussion about the process, and try it for a while and see if it needs work. This issue is the start of that discussion. Thanks TWG for starting the discussion!

One specific comments about the ideas in #2:
- in item 3 & 5, the announcements should be made to the "Core" group on groups.drupal.org I think, and maybe elsewhere? Need to figure that out.

tizzo’s picture

Yeah, I had been thinking maybe g.d.o posts to "Core" and "Governance" groups at the least.

tizzo’s picture

I think the idea of setting a time-box on the decision is a great idea.

Also - per @jhodgdon's ideas in #2381209: Technical Working Group and/or its charter broken? I think we should add:

  • Community members would have the opportunity to "appeal" the decision for a week (I'm not sure what "appeal" would mean, or whether this step is necessary actually).
  • One week after RTBC the Coding Standards Committee, unless there was a successful appeal, would finalize their decision and update the relevant standards documentation.

[Edited: linked to the wrong issue]

tizzo’s picture

dawehner’s picture

@tizzo
Oh I'm sorry, this was a crosspost. To be clear, I was referring more to the description of the status quo.

Create an issue in the TWG queue
The issue is added to the list of changes under consideration in a given batch (issues open before say a week prior to the TWG monthly meeting)
An announcement is made of the coding standards changes under consideration with an end date (say 60 days?)
If a clear majority of commenters disagree with a policy change it is dismissed
At least one core committer needs to agree to the change
At least one contrib module maintain needs to agree to the change
If no clear majority exists but core and contrib maintainers have advocated for the change, the issue will be automatically escalated to a private decision by the Techincal Working Group
The next monthly meeting of the TWG meeting the issue will be evaluated using the above criteria
An annoucement of all changes for that session will be made
What I'm optimizing for: meeting needs of core and contrib, timeliness, community driven process, clear anouncements, preventing deadlock, ensuring TWG can keep up without taking too much control.

I like the general process, like taking into account core committers, contrib maintainers etc. Also the additional announcements is crucial, given that this is lacking at the moment.
One suggestion, we could have something like a twitter account which pushes out new issues which needs discussion. The current visibility of the TWG queue for itself is broken and is IMHO the most broken part of it.

On top of that its not clear for me, when the opinion of different alternatives is formed. I guess there will be a timeframe for discussion between 1. and 2., where the community can come in and form an opinion? I doubt
that the TWG itself always need to have an opinion about problems.

bojanz’s picture

+1 to #2 and #6.

Announcing the discussion before it happens, and time boxing it is essential. Giving contrib a formal voice is appreciated.

pfrenssen’s picture

I also think mainly lacking a formal way of concluding these issues is the problem. Often there is a flurry of comments at the start of the issue, and then it just dies down.

Can we perhaps use something similar to the PHP RFC process? A proposal is made and announced, and then a period of X weeks starts in which people can comment and the proposal can be refined. After these X weeks there can be a window of Y days / weeks in which people can vote. Separating the discussion from the voting is important I think, so that people can change their minds if a proposal is adapted after receiving comments.

We can put some thresholds in place to avoid controversial proposals from being accepted, like a minimum of 70% of votes in favor, and a minimum of 5 voters for it to be accepted.

jhodgdon’s picture

I am not sure that Drupal is a democracy, or that we really want it to be a "one person one vote" project. We have "maintainers" for a reason...

I also don't think that, as a practical matter, a voting threshold would work here. Check out the past/existing open coding standards issues -- it is a struggle to get people to even comment or express an opinion. Not many people care.

For that matter... maybe it would be better to appoint a committee to make the decisions. Ask for community input and discussion for a certain period, and then let the appointed committee decide. That is basically what we do about Core and Contrib patches -- there are Maintainers, who make the final decision to commit patches after community review and comment. Maybe we need to have Coding Standards Maintainers too? Just a thought...

xjm’s picture

Just a couple thoughts on the practical aspects of adopting new coding standards:

  1. I think one essential thing is missing from this process for the longterm, and that is mention of testability and testbot support. See in #1518116-86: [meta] Make Core pass Coder Review. While having coding standards generally improves DX, having no way of learning them other than reading 318 and 1354 leads to frustrating contributor experiences. Therefore, once the new testing infrastructure supports coding standards checking, I think part of the process for adopting a new rule needs to be adding a test for it. No test, no standard.
  2. We are also lacking explicit criteria for what coding standards we adopt. Off the top of my head, this includes things like:
    • Legibility and learnability.
    • (For documentation) Clarity for second-language English speakers.
    • Internal consistency.
    • Support for IDEs.
    • Support for the API module for api.d.o.
    • "Future-proofing" code to avoid fragility and regressions. (Simple example: trailing commas in multi-line arrays.)
    • Etc.
  3. I think a lot depends on the scope, also. Few people care whether we indent the second line of our @todo two spaces. On the other hand, if there were a move to adopt PSR-2 or something, that would be a whomping big deal and a lot of people would probably care.
  4. There are release management considerations as well. What expectations do we have for core adopting a new coding standard once it's accepted? 8.0.x-dev is closed for pure coding standards cleanups (edit: unless they are docs-only), so this also implies that we will not adopt new coding standards before 8.0.0 if they would require changes to the existing codebase.
  5. Does it make sense to recommend new standards to contrib before core is compliant? IMO it doesn't, so whether or not core is compliant is also a consideration in how we roll out changes to the community. Edit: This is also related to #1.
mile23’s picture

To @xjm's point in #13: No test, no standard... Best way to go.

See #2415441: Automate finding @covers errors which checks @covers annotation for correctness (that it's syntactically correct, and that it declares coverage of something that exists. It fails tests that are marked up incorrectly.

It adds a PHPUnit listener, called DrupalStandardsListener, which could be expanded to check for other things as well, but it's scoped to PHPUnit-based tests, so maybe not as useful as a standards phase in the testbot script.

But it's there.

jhodgdon’s picture

Not all coding standards can be checked for automatically in a test. For instance, we have a "standard" in the Drupal project in general that our documentation should be accurate and concise. And we have a standard that says the description after @param should describe what the parameter is used for. So we can easily test to see that there *is* a description, but to verify that it's accurate and descriptive is not possible.

And we are now documenting Event constants by adding @Event to them, but I don't imagine there's a very straightforward way to detect that a constant in any given class is an Event constant. Yet if we hadn't adopted that practice, we wouldn't have this nice new useful list at:
https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group...
So we added that tag to node/1354... under this guideline for standards, we wouldn't have been able to do that at all.

Also, as I'm sure you're aware, even for standards that can come with a test, some of the proposed tests may take too long to run and be rejected from being part of the testing scheme. And it's also problematic to add a test that will fail on the current code base; yet this is normally the case when a new standard is adopted: it's not being followed until the code is updated.

So... Making a blanket statement that we will no longer adopt standards that don't come with tests is not really workable and I think is too high of a bar. How about just saying that "If the standard is feasible to test, a test must be written before the standard is adopted"? Or something to that effect.

jthorson’s picture

While I feel it's both fantastic and important that this discussion is happening (thanks, Howard!), I'd like to suggest that we limit the amount of hard process and bureaucracy that gets built into such a process.

While the TWG charter was being drafted, my interpretation was that the role of the TWG was two-fold:
i) To document, curate, and maintain the existing technical policies within the community, and
ii) To act as a mediator or arbitrator in cases where the community was not able to come to a consensus on a technical policy decision.

Essentially, I equate the first role to essentially that of a technical policy 'librarian', and the second role as being a dispute resolution body for technical conflict, just as the Community Working Group is a dispute resolution body for personal conflict within the community.

What I would want to protect against is the perception that the TWG becomes a technical policy 'gate' ... I believe it is desirable and healthy to have the community itself initiate, debate, and come to a consensus on technical policies it wants to implement; and in these cases, the TWG should be little more than a registrar of the community-initiated policy.

Now in many cases, consensus is not reached ... or the issue itself simply grows stale and dies.
i) For the former situation, lack of clarity around who has the authority to make a final decision means that the issue goes nowhere.
ii) For the latter situation, the lack of clarity on the next step on how turn something from a 'discussion' into a 'policy' means that the issue simply sites and stagnates.

These are the two gaps that the TWG is now well positioned to fill ... but I would suggest that we *don't* want every single coding standard proposal dumped into the TWG queue for the duration of the community debate; I think we only want to see the 'mature' discussions that are ready for TWG involvement ... and further, the TWG should not be heavily involved until after a policy has had at least an initial round of community involvement/feedback.

These opinions draw heavily on a couple of items within the charter:

The TWG does not necessarily author and maintain these policies itself, but merely ensures that they exist, that they are well maintained, and that they are successful.

The TWG’s focus is on enablement and support of the Drupal community, not the core Drupal project. The TWG does not drive the technical roadmap or strategic decisions for Drupal core or contrib, including (but not limited to) policies on backwards compatibility or release timing and management; excepting those which have been sanctioned as falling within the TWG’s scope of responsibility (e.g. maintaining the Drupal Core coding standards).

The TWG may not enforce a newly created standard or policy until inclusion of that policy into the TWG’s scope of responsibility has been officially approved and sanctioned by Dries Buytaert and/or his designate(s).

mile23’s picture

@jhodgdon #15: We can work towards using a tool like phpcs with a coding standard to fail patches that don't match our standards. That way we improve the chances that the content itself is right and accessible.

Then, when that brick wall is in place, we can discuss further standards as a patch/review process within TWG project and also to the phpcs standard or other tool, because patches will fail and people will take issue with the fact that the brick wall doesn't meet their needs.

Re: @Event: "under this guideline for standards, we wouldn't have been able to do that at all."

No, I'd say we need a standard that exists outside the tool so that people can read it and it's the spec for the tool. But if it's not a test, then it's less likely to be enforced. So you could use a code sniffer to test for a typo like @Events, and do static analysis to see if @Event annotates a const, and check whether there is any other documentation accompanying it, because who wants an undocumented event? That way when someone does bother to annotate, we can begin to check it for accuracy.

So a process might look like this:

  • Determine the need for a standards change.
  • Document the change in an issue under TWG project, sort of like an RFC.
  • Create an issue to change the code sniffer, mark as postponed on the TWG issue.
  • Hammer things out in TWG.
  • Change the code sniffer when there are specs to work from.
  • Use the code sniffer in the CI process.

Using 'code sniffer' here as a stand-in for whatever tool we end up using. In the case of @covers it's DrupalStandardsListener. :-)

jhodgdon’s picture

@Mile23 - all I'm saying is that there are possibly some standards that do not lend themselves to tests, so I would be against a blanket statement that all coding standards need to come with tests, and rather state it as a "usually" or something like that.

Crell’s picture

tizzo asked me to weigh in here, but I don't think I've much to add beyond the very good points that have been made above.

In short, the key points for me are:

1) Announcement on g.d.o/core so it goes out to the various distribution channels.
2) Time-boxed discussion.
3) Some written guidelines for how we should collectively evaluate a proposal. Those don't have to be tightly proscriptive, but we should have some guidelines to ensure we're all evaluating a proposal on vaguely the same merits. Or at least trying to.
4) Tests "where possible" seem fine, and gives us something to make a patch out of. I agree with Jen, though, that we shouldn't get too hung up on something that is in many cases quite hard to lint. (If it's easy to lint, totally we should do so.)

I disagree with jthorson, though. Why don't we want all such discussion centralized in the TWG queue? It being in the TWG queue doesn't necessarily mean that the TWG is now Owning The Process And Planning To Enforce A Decision(tm). It means that there's a known, discoverable place for such discussion and it can transition from informal to formal easily and readily as the TWG determines it's appropriate to do so.

jthorson’s picture

#19:

I agree with having a known, discoverable place for the discussions, in a centralized location ... but am not sure that the TWG queue is the best place for that. I would suggest that the TWG queue is actually *less* discoverable than the core queue for core coding standards discussions ... but in the end, feel that neither queue is a perfect fit for hosting the discussions.

My main concern is related to the volume of coding standard proposals, which is only one small piece of the TWG mandate; and the risk of this drowning out other more immediate technical policy discussions and/or technical conflicts in the queue. I'd also like to avoid creating the perception that simply posting a coding standards proposal to the TWG queue automatically means that the TWG will give that proposal attention ... I believe the onus should still be on the author to socialize their proposal within the community and build a base of support, and the TWG should not become involved (outside of perhaps assisting with notification of a new proposal looking for input) until the proposal has at least receive an initial reaction and round of debate within the community.

I would support another option (which was suggested during our TWG call yesterday) that the coding standards reside in a dedicated queue of their own, and that the TWG maintain this in addition to the TWG queue; as I believe this addresses both of the items I've identified above.

jhodgdon’s picture

+1 to jthorson's idea of having a dedicated queue for standards issues. This would also remove the appearance of it being the TWG who is primarily responsible for *making the decision*, as opposed to *overseeing the process* and only in rare cases probably intervening with an escalation.

jthorson’s picture

Just for clarification ... it was either Howard or Alex who suggested it ... I'm just a supporter. :)

Crell’s picture

A dedicated queue sounds fine to me.

mile23’s picture

So should there be a project called coding_standards?

jhodgdon’s picture

RE #24, ... if/when this is adopted, yes, that would be the way to have a dedicated issue queue.

lewisnyman’s picture

Great to see this process being discuss, we sorely need a way to evolve the CSS coding standards, as they are still quite new and have not covered everything. We are curretly using CSSlint to enforce the CSS standards that are parseable.

jhodgdon’s picture

So... Where are we going on this? I think there's pretty good agreement on what the process should be... What is the time frame for adopting/formalizing and putting it into practice?

We have quite a few coding standards issues sitting in this queue (some have been sitting around for years), so it would be a great and wonderful thing to have a way forward for them. Thanks!

tizzo’s picture

I agree, I think we're very close to consensus on this. I'm drafting an "official" workflow recommendation on behalf of TWG which I'll post this week based on the above discussion. I just want to get sign-off from Alex and Jeremy on it first. I'm hoping to do that by our meeting Friday.

Thanks for the nudge! :-)

jhodgdon’s picture

Excellent, thanks! I guess at that time we'll also need to set up the project and move the existing coding standards issues over there.

tizzo’s picture

Issue summary: View changes
Status: Active » Needs review

Based on community feedback the TWG has spent some time drafting the proposed workflow that I have added to . We'd love your feedback but I think this is pretty close to what we were all saying.

You'll not that the language around the coder module tests was made a suggestion rather than a requirement for changes partly because, as jhodgdon pointed out, not all standards can be tested for, but also partly because coder module is an independent project with independent (though highly involved) maintainers and do not feel that we can enforce that as policies change commits happen there. We can just push issues to write patches so that it's easy for the coder folks to keep up as they see fit.

I'm marking needs review for final feedback.

tizzo’s picture

Issue summary: View changes

Modified the description to clean up a couple of stray characters.

David_Rothstein’s picture

This looks really great but I don't understand this part (particularly the first point):

2. At least one core maintainer (someone in MAINTAINERS.txt) and at least one contrib module maintainer (listed as a maintainer for at least one full project) needs to agree to sponsor the change.
3. The issue can now be tagged "needs announcement for final discussion" it becomes eligible for consideration.
4. The coding standards committee use their discretion to decide which and how many of the "needs announcement for final discussion" issues to announce and tag with "final discussion".

The first point seems really arbitrary to me, and looks like it would be a high enough bar to annoy people but a low enough bar not to have any real benefit...

Why not treat this more like any other issue queue:

  1. When an issue is first posted for discussion it's set to "Active".
  2. When the discussion has advanced to the point that people working on it feel they have a concrete proposal that's ready for community review, they set it to "Needs review".
  3. If the coding standards maintainers (or anyone else) look at it and see that it isn't fully-formed and/or hasn't had any real discussion yet, they can quickly change the status back to "Needs work" as soon as they see it.
  4. Otherwise the coding standards maintainers can look at it more carefully and decide whether to announce/tag for final discussion.
jthorson’s picture

From my personal perspective (individual, not TWG role), I think the sponsor requirement in #2 helps by putting some up-front accountability on the individual proposing any given coding standards change to socialize that idea with others before it gets posted to the queue.

The bar is 'anyone in Maintainers.txt' (versus 'core committers'). There are currently over 175 entries in that file, and even accounting for duplicates, that should still leave us with over 75 potential sponsors within the community. (Just a guess; I didn't count unique entries.)

I would also hope that the 'sponsor' role provides us another body who cares enough about the issue to assist with facilitating discussion and/or developing some momentum and support behind the proposed change ... though in practice, I realize this will be hit and miss depending on the individual.

David_Rothstein’s picture

People should definitely discuss/refine their idea with others and get support for it before putting it up for official review, but why not just say that directly?

I just don't see why being a core maintainer (or a contrib maintainer, of which there are, like, thousands) makes someone more qualified to help push forward a coding standards change, compared to someone who is an active Drupal contributor but doesn't happen to maintain anything officially... There are a fair number of people in that latter category, I think.

tizzo’s picture

So requiring a core maintainer to sign off on the change was to appease the concerns raised by folks like catch in comments like the one he made on #1929534-3: Technical Working Group Charter (DRAFT) where he said:

...as a core developer I do not want to be expected to implement standards defined outside of the core process.

Given that coding standards dictate what can be sweeping changes to the Drupal code base the idea here is to ensure that you have someone deeply involved in that process on board. The idea of adding a module maintainer was to try to ensure there was symmetry and that the ecosystem outside of core has some representation as well. That second part is supposed to be a low bar and if you're not maintaining a module the coding standards (presumably) have much less of an effect on you.

Crell’s picture

Having a process change sponsored by someone with a clear vested interest is not a new concept. It's been used in most of the groups I've been involved in over the years, and the ones that didn't had a much harder time with signal/noise ratio. PHP FIG, for instance, requires at least 2 voting reps to sponsor a proposal before it can be brought to an entrance vote, which is the "yes, FIG will even consider having this thing".

"Anyone in maintainers.txt" sounds like a clear and justifiable definition of "vested interest".

David_Rothstein’s picture

@tizzo, is that concern not covered by the part later on? I mean here:

  • The announcement will be made via TWG Twitter account, groups.drupal.org/governance, groups.drupal.org/core, and "this week in drupal core"
  • Issue is considered with the following guidelines:
    1. If a clear majority of commenters disagree with a policy change it is dismissed
    2. If no clear majority exists but core and contrib maintainers have advocated for the change, the issue will be automatically escalated to a private decision by the coding standards maintainers
effulgentsia’s picture

Why not treat this more like any other issue queue

Actually, I think the proposal does this, per below.

1. When an issue is first posted for discussion it's set to "Active".

Yep. So far so good.

2. When the discussion has advanced to the point that people working on it feel they have a concrete proposal that's ready for community review, they set it to "Needs review".

For normal issues, "Needs review" is just a request for someone to review, not necessarily a maintainer. So, more like peer review. Which I think would be good here as well, and which the proposal as-is allows for, though perhaps could be more explicit. Someone could post an idea that's still rough, set to "needs review" to get someone who cares to give feedback, and iterate on it until someone feels it's ready for an announcement for final discussion.

3. If the coding standards maintainers (or anyone else) look at it and see that it isn't fully-formed and/or hasn't had any real discussion yet, they can quickly change the status back to "Needs work" as soon as they see it.

For a normal issue, there'd be a step here for setting the issue to "reviewed & tested by the community" in order to signal to the coding standards maintainers that the participants on the issue believe it to be ready. But for normal issues, "the community" is just the people who happened to find the issue without much advertisement needed. Whereas for coding standards changes, we want "the community" to include more people, so we want an "announcement for final discussion" step. But we don't want to flood the community with announcements of things that aren't sufficiently vetted yet. So, we need a way to signal to the coding standards maintainers that the participants believe the issue to be ready for that step, which is what the "Needs announcement for final discussion" tag aims to provide.

The first point [At least one core maintainer (someone in MAINTAINERS.txt) and at least one contrib module maintainer (listed as a maintainer for at least one full project) needs to agree to sponsor the change] seems really arbitrary to me, and looks like it would be a high enough bar to annoy people but a low enough bar not to have any real benefit...I just don't see why being a core maintainer (or a contrib maintainer, of which there are, like, thousands) makes someone more qualified to help push forward a coding standards change, compared to someone who is an active Drupal contributor but doesn't happen to maintain anything officially... There are a fair number of people in that latter category, I think.

I think this requirement is consistent with the proposal for scaling core decision-making in general (#2457875: [policy] Evolving and documenting Drupal core's structure, responsibilities, and decision-making), and would serve the purpose of filtering out many insufficiently vetted proposals from the list that the coding standards maintainers need to monitor. While it's true that there are great reviewers not in that list, I'd be suspect of any coding standards change proposal that can't get a +1 from at least one person who is in that list, and can't see myself ever wanting to approve a coding standards change that can't get at least one core subsystem maintainer and one contrib maintainer to like it, even if lots of other excellent reviewers all do like it. Given that this requirement would effectively exist prior to the approval stage anyway, I think there's benefit to applying it prior to announcement, to minimize announcing things that aren't otherwise approval ready.

...as a core developer I do not want to be expected to implement standards defined outside of the core process.

Re-reading this, I'm concerned that the current issue summary's proposal doesn't meet this goal. Because a core subsystem maintainer can +1, a contrib maintainer can +1, the change proposal can get announced with no one complaining during their 2 week window, and a coding standards maintainer can "approve", and bam, there's now a new coding standard that core needs to follow, but no core committer explicitly agreed to, which means it didn't follow the "core process". So, I'd like some feedback, especially from core committers, on how to address that. Some options (these are mutually exclusive, not additive):

  1. Require that prior to approval, a core committer must +1.
  2. Once an issue is "approved" by the coding standards maintainers, move it to the core queue as RTBC. Wait for a core committer to move it back to the coding standards queue with a comment saying "yes, make it so". (Essentially the same as the previous option, but using the core RTBC queue as a medium, since that's where other core decisions get made).
  3. Add one or more core committers to the coding standards maintainer team, and leave it to them to mark an issue as "approved".
  4. None of the above, and instead decide that it's ok for the coding standards maintainers to approve a change that doesn't have explicit sign-off from a core committer.
webchick’s picture

I think any of those sound fine, but I think #2 most directly addresses catch's concern, which is that there are far more people at any time looking at the Drupal core queue than there are looking at the coding standards queue. In fact, maybe the move to the core queue could coincide with the "ready for wider feedback" step, but then it's a bit weird to leave issues at RTBC for 2 weeks.

tizzo’s picture

Issue summary: View changes

Based on the most recent feedback and in a better attempt to address @catch's concerns - we are updating this proposed policy to add a step. After the coding standards group approves a proposed change the issue will be moved to the Drupal Core queue and marked RTBC. A core committer will then sign off on the issue and pass it back to the coding standards queue for announcement and documentation updates (or will veto it, mark it needs work, etc).

The description has been updated and we are soliciting final feedback as I most of the concerns raised to date have been addressed.

David_Rothstein’s picture

I think this requirement is consistent with the proposal for scaling core decision-making in general (#2457875: [policy, no patch] Evolving and documenting Drupal core's structure, responsibilities, and decision-making)

I really think the opposite, actually:

  1. The policy discussed there is very explicit that the purpose is to make sure maintainers have an opportunity to sign off on changes, but that changes won't be held up forever waiting for a maintainer to look at it. What's being proposed here is to wait forever for a maintainer to approve.
  2. The policy discussed there is explicitly about making sure decisions happen based on areas of expertise (signoff from subsystem maintainer X on a change that significantly affects subsystem X). However there is no reason that being in MAINTAINERS.txt makes someone more of a coding standards expert than anyone else.

While it's true that there are great reviewers not in that list, I'd be suspect of any coding standards change proposal that can't get a +1 from at least one person who is in that list, and can't see myself ever wanting to approve a coding standards change that can't get at least one core subsystem maintainer and one contrib maintainer to like it, even if lots of other excellent reviewers all do like it.

It's certainly true that if lots of maintainers disagree with a proposal it won't get in. (I'm not sure what happens if they're apathetic about it and don't comment one way or another.)

But the point isn't about whether they agree with it, it's about the requirement that you need to find one of them to "sponsor the change". What is the effect of this on someone who is a new contributor (or even not so new but not part of the "in crowd")? I think this is an intimidating requirement for a new contributor who wants to propose a coding standards change. (Or perhaps intimidating for some, an invitation to spam the contact forms of everyone in MAINTAINERS.txt for others :)

Even as an experienced contributor I am not sure what I'd do in this situation. Let's say I post a proposal for a coding standards change, it gets some good feedback and people seem to like it, but no one from MAINTAINERS.txt happens to chime in. What do I do next? Pick a name at random from MAINTAINERS.txt and ask them to take a look?

David_Rothstein’s picture

In the interest of a concrete proposal, I propose changing this bullet point:

2. At least one core maintainer (someone in MAINTAINERS.txt) and at least one contrib module maintainer (listed as a maintainer for at least one full project) needs to agree to sponsor the change.

To something like this:

2. At least two other contributors need to review the proposal and agree to sponsor the change.

This preserves the fact that the "needs announcement for final discussion" tag is essentially a "preliminary RTBC" state, and therefore needs a couple people to review the issue before it gets marked as such (as is normally the case with RTBC issues).

I think that is fine for now; anything more complicated than that looks to me like it's solving a problem that hasn't been shown to exist yet. If it turns out in the future that way too many issues are being given this tag that clearly aren't ready yet, the policy could always be revisited then. To be honest, I don't think it will be a big problem. As maintainer of Drupal 7 core (which I'm pretty convinced has more "marked-RTBC-way-too-early" issues than any other issue queue on drupal.org) it can definitely be annoying sometimes, but dealing with those issues is quick and usually not that much of a maintenance burden. I think for a coding standards issue queue it is likely to be a lot less.

David_Rothstein’s picture

7. The issue will be moved to the core queue and marked RTBC for core committer signoff

I think this needs more clarification regarding which core committers... Any of them? Or if a coding standards change affects more than one branch, would it need signoff from maintainers for each branch that it affects, or still just one?

The MAINTAINERS.txt part probably would need similar clarification (what branch it applies to) but again, I think most of that should be removed.

tizzo’s picture

Issue summary: View changes

There were two motivations in requiring a core maintainer to give a +1, the first was to ensure that issues under consideration were likely to be incorporated and second to move some of the onus of verifying that this was a supported and supportable change onto the person submitting the request. Now that we have a provision for moving these issues through the core queue, I think the one issue is resolved and I think David is probably right that the coding standards committee can probably quickly dismiss the bad ones or ask for more support. In the interest of getting this process up and running, we'll run with David's suggestions just asking for two +1's from "active community members" and see how it goes.

We also changed the tagging a bit to make it more descriptive and intuitive but it is otherwise unaltered.

Any objections to the new policy as it is now written?

David_Rothstein’s picture

Thanks, looks good! I'm not sure "active community members (contrib module maintainers, active patch contributors, etc)" is well-defined, but I doubt anyone will fight about it either :)

I'm still curious about #43 also, although I guess that can actually just be worked out in the core queue (it's OK for the policy here to be a little vague about core process).

Overall, +1 RTBC from me.

drunken monkey’s picture

So, what's needed to finally accept this and start going through all those proposed and mostly resolved coding standards update suggestions?
(I guess we lack a process for making changes to the process for making changes to the coding standards, too …)

tizzo’s picture

Status: Needs review » Fixed

Actually it's funny that you just asked because the first official meeting to start reviewing coding standards issues is scheduled for today! We'll be working through the existing issues in order of severity identifying issues that are eligible to feed into this process.

I'm marking this issue fixed and any new issues raised in relation to this process can be created in a new issue.

Finally, progress!

tizzo’s picture

I have added the workflow here as the description in new project for all coding standards issues.

chx’s picture

This needs to be posted to gdo/core

dawehner’s picture

chx++

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

Also, this should probably be documented in the handbook and/or governance project somewhere? And https://www.drupal.org/project/coding-standards (linked in the summary) seems not to exist.

Thanks @tizzo!

xjm’s picture

Ah, the project is actually https://www.drupal.org/project/coding_standards -- and the policy is documented on its project page. Sorry for the noise. :)

tizzo’s picture

I totally agree - I got the link wrong (obviously) and also we need to add a page under the twg section of the governance handbook. Adding that to my todo list.

tizzo’s picture

Issue summary: View changes

Fixed the bad links. Sorry about that, my mistake.

xjm’s picture

Issue summary: View changes

Adding a note that the policy is also found on the project page.

anoopjohn’s picture

Issue summary: View changes

Corrected link to the coding standards page in the issue summary.

avpaderno’s picture

Issue tags: -#codestandards