Problem/Motivation

Drupal core governance defines the role of a "Subsystem maintainer":
https://www.drupal.org/contribute/core/maintainers#subsystem

The responsibilities for the role are the same regardless of whether the subsystem is a module, theme, or in core/lib. However:

  • MAINTAINERS.txt lists modules and themes completely separately from "subsystems", which makes it seem like they have separate, undefined maintainer roles.
  • The core issue queue accordingly separates modules and themes from other "systems", which results in a confusing, not-quite-alphabetic form field.

Proposed resolution

  1. Merge modules and themes into the subsystem list in MAINTAINERS.txt.
  2. Standardize the capitalization.
  3. Remove the unnecessary qualifiers "system", "module", and "theme". In most cases, these words are noise.
  4. Where necessary, clarify the component name (e.g., "Entity" becomes "Entity API" which is what we call it anyway).
  5. Remove any totally duplicated components.

Not in scope

Remaining tasks

The attached patch should apply to 8.2.x and 8.3.x. It will need to be backported to 8.1.x. The -do-not-test.patch includes the substantive changes after reordering the subsystems and standardizing their headers.

The following tasks should be done in separate Drupal.org issues once MAINTAINERS.txt is updated, but are not in scope here:

  1. Update the core issue queue component list for these same changes.
  2. Programmatically update Drupal core issues to those new components. Possibly retain old data e.g. as issue tags for historical purposes and to support future tools improvements.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

Missed one.

xjm’s picture

Issue summary: View changes
Parent issue: » #2660144: [Plan] Update core components
alexpott’s picture

Where should system module and base system issues be filed then?

xjm’s picture

One more.

xjm’s picture

Removing the Base and System removals because this also probably needs a governance discussion and @alexpott pointed out the issue migration for them is not 100% straightforward.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

When I first read this issue I expected more of a reworking. Now I see what is changing here this looks like a good first step because nothing is really changing. It is just removing duplicates after removing the meaningless "system", "module", and "theme". Wrt. to system module and base system +1 to not changing that here.

However, we do the re-organistion proper, incremental change without an agreed destination is a big problem for changes to how we organise maintainer.txt. I think this is one of those issues where the people empowered to make decisions should just choose the list of components and make it so. Why? Because with each incremental change we'll be asking people to get used to a new paradigm which is a barrier to filing issues correctly. And we someone learns the new way , we change it again.

Given the first part of this comment and this patch is not changing anything beyond sorting and de-duping I think this is rtbc. I also look forward to a completely alphabetical component list which would be a benefit of this.

tim.plunkett’s picture

I disagree with this, but I'm having a hard time putting it into words. Going to come back tomorrow.

xjm’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs review

We end up with an 'Email' component with one maintainer and a 'Mail' component with no maintainer. Not that 'Email module' and 'Mail system' is much easier, but at least in context you could tell that one's higher or lower level. Changing it to 'Email field' and 'Mail sending' or similar might help but for me this makes it harder to identify what's what.

Moving back to CNR for that and Tim's comment.

For me there's also a small concern with core/lib vs. core/modules vs. core/themes - the existing list is not that far off an ls of the directories (at least it probably started as that for modules then the other bits got added later), and we lose that by making it alphabetical.

xjm’s picture

We do not have 1:1 mapping from core/lib namespaces to components (which is good from my perspective because it would be unusable if we did). And there are some subsystems that do and/or should include code both in core/lib and modules. So for me removing the coupling between specific code paths/implementations and subsystems/governance is a good thing since it is not really entirely reliable anyway.

YesCT’s picture

I just ran into this trying to fill out sign offs needed for #2788253: Plan for DateTime Range experimental module

xjm’s picture

I think the Email module needs to be called Email Field, because it does not let you send emails. That would be a separate followup I think (including proposing a name change in the UI of that module as well). Same is actually probably true of (e.g.) Number (regardless of whether it is listed with the word "module" or not in this file).

For clarity (in case this was @tim.plunkett's concern), this patch does not make any changes to core governance. It just reorders the file to remove some internal/implementation details. Edit: it will hopefully give us some room to make improvements to the issue queue in the future, but such changes will be disruptive and so scheduled well ahead of time.

I also think we have some long-outstanding governance work to figure out exactly who is responsible for what parts of core/lib, but that's a bigger problem that needs to be addressed separately.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

@catch, would you be comfortable with the reordering first, but retaining "system" and "module" in the individual headers until we discuss it separately? For me removing those words is an important part of making the issue queue sane, removing redundancy, and unblocking some other work, but the reordering is the pain to reroll that I'd like to not have to do too many times. Changing the labels OTOH is a regex.

tim.plunkett’s picture

There is a difference between the code in core/lib and core/modules (or theme).
Modules for the most part are self-contained, and when they rely on other module code, they have explicit dependencies.
Therefore, usually changes to that module are also self-contained.

Changes or improvements to subsystems are often far-reaching and pervasive.

It is nice to be able to tell at a glance, "Oh this is a module" vs "this is an entire subsystem".

xjm’s picture

Thanks @tim.plunkett!

In our governance policy, modules are already considered subsystems. (The choice of this term over "component" was based on governance research that @webchick did and eventually other committers agreed to use that term.)

tim.plunkett’s picture

If we've already muddled that distinction beyond repair, then I withdraw my objections to this issue. I think #17 is a better scope for this issue, but the direction of the patch is inevitable.

dawehner’s picture

There is a difference between the code in core/lib and core/modules (or theme).
Modules for the most part are self-contained, and when they rely on other module code, they have explicit dependencies.
Therefore, usually changes to that module are also self-contained.

Do I understand you correctly that in order to understand the scope of a change within a core/lib subsystem it would actually help when we think about the dependency of them underneath each other? For example we could have level 0 components like database, everything in component, that don't depends on anything, then level 1 which depends on those, like state, key_value, cache. Even I think it could be pretty hard to figure out these dependencies, it could help us to not only understand changes, but actually scope changes in the first place.

webchick’s picture

To address @tim.plunkett's concern, I would advocate the use of tags, essentially flipping the use of the two tools.

The component list is something exposed to end users in the bug report form. Sometimes to really new end users. So they're going to choose from that list when trying to decide what's the most appropriate place to put their bug report. Because of the overwhelmingness of the current list, many, many people just give up and choose "base system" or "ajax system" (since it's first), which is both not helpful to them, and also not helpful to people in the actual affected subsystems.

Tags, on the other hand, are not exposed to end users. You need to know the special words to use to use that list, which by definition you will not until you're a community insider. End users are never, ever going to know to tag something "VDC" to get it in front of views maintainers.

Therefore, give the end users the "bucket" to get to the approximate appropriate people ("Views"), and let the people within that bucket who know the system inside and out apply tags to help them self-organize better ("Views module" vs. "Views UI" vs. whatever). Then we don't lose the separation of lists that make it valuable to maintainers, but we don't force end users to make this distinction, which they can't.

webchick’s picture

Additionally, I don't really understand the desire to have this level of granularity even from a maintainer POV. Two (or four, or six) heads are better than one. Rather than the SQLite maintainer and the PostgreSQL maintainer and the MySQL maintainer all working individually in their own isolated silos, why is it not more valuable for them to be able to easily get a list of all "Database" issues, some of which may impact multiple database engines? With fewer subsystems, we can actually start to build teams around these subsystems, so people can like, go on vacation and stuff. :)

xjm’s picture

Assigned: Unassigned » xjm
Issue summary: View changes
Status: Needs review » Needs work

Thanks @webchick and @dawehner

Just want to make a quick note here that the actual migrating of components etc. is not in scope in this first issue (because we really want to focus on just making the governance as clear and accurate as possible before going onto any changes) but these are some really great ideas for subsequent steps, so I'll make sure to reference the discussion in subsequent children of #2660144: [Plan] Update core components. The tag suggestion in particular is a neat idea for solving a few problems without needing to be wholly blocked on DA resources etc. Adding that as a note in the remaining tasks, too.

I think #21 also sounds like an interesting strategy to help us understand what our subsystems are or should be. The "base system" component is like the level 0 example probably. Maybe we can do that as part of #2660144: [Plan] Update core components as well.

@catch, @webchick, @Cottser, @Dries, @alexpott, @effulgentsia and I discussed today how we can make this initial patch better, so I am going to update it with some additional fixes proposed in that discussion.

xjm’s picture

Status: Needs work » Needs review
FileSize
17.37 KB
5.26 KB

New patch, not rerolled from the others.

It turns out that email.module got removed a long time ago (both from the codebase and the issue queue) and the functionality is provided by the Field API itself, so we don't need to worry about renaming that. I've just removed it. However, both "Text" and "Link" were also confusingly ambiguous, so I added the qualifier "Field" to them. We might consider a followup to discuss whether those should be their names in the core UI as well (though maybe not since in the UI they are in a "field" package).

xjm’s picture

Oops, wrong patch above. The -do-not-test is correct though.

The last submitted patch, 25: maintainers-2785891-25.patch, failed testing.

catch’s picture

New patch helps a bit.

@tim.plunkett that's the case for some modules like comment. But changes to Views can affect multiple modules, which don't have an explicit dependency due to the plugin system and optional configuration. Similarly there are core subsystems like transliteration which are a lot more self-contained than several modules.

I agree with #22 - I'd like to see a much shorter component list on the issue form so there's more chance of things being in the right place (even if that place is bigger), and then trim those lists down via tags.

With the database example we already tag things 'sqlite' or 'postgresql' - because it'll be a Views query or a comment patch failing on case sensitivity in postgres or simpletest failing only on sqlite as often as it's a problem with the drivers themselves. But sometimes issues in modules get filed against the databases, because it's not really clear if it's a subsystem or a topic - and unless you understand what the bug is already you might not even know.

YesCT’s picture

I'm not sure how I feel about removing the word module from actual modules; I think it can be a helpful hint. Reordering and consolidating is a good idea.

Concern, but not to block this:

  1. +++ b/core/MAINTAINERS.txt
    -- John Albin Wilkins 'JohnAlbin' https://www.drupal.org/u/johnalbin
    +Provisional membership: None at this time.
    

    Provisional component/module uh, subsystem, maintainers I hope would be listed under the subsystem they are provisional for.

xjm’s picture

@YesCT, in our governance policy, there is no provisional membership for subsystems. You become a provisional subsystem maintainer by doing the work in the queue. :)
See here: https://www.drupal.org/contribute/core/maintainers#subsystem

xjm’s picture

Ah and regarding:

I'm not sure how I feel about removing the word module from actual modules; I think it can be a helpful hint.

@Cottser, @webchick, and I talked about the idea to remove the word "module" (the inspiration for this issue). One of the things that makes finding a component overwhelming is the sheer textual load of the field. Usability research shows that the fewer words a user has to deal with, the better they understand a UI. (We made lots of changes to remove text from Drupal 8's UI for this reason, for example.) By removing the word "module", we reduce the text in the list of component names by more than 1/3. Removing the word "module" also allows removing several redundant entries in the file, as well as giving us a path forward to consider combining subsystems into groups based on their function and maintenance rather than their implementation.

xjm’s picture

I'd like to see a much shorter component list on the issue form so there's more chance of things being in the right place (even if that place is bigger), and then trim those lists down via tags.

Yeah. For me that is a next step, after this issue, because it requires actual governance changes.

xjm’s picture

Can we talk about anything that's listed as out of scope in the summary on here instead? #2660144: [Plan] Update core components I want to not fragment the discussion, since this patch is intended to just improve documentation of the current governance rather than to make any changes.

dawehner’s picture

Where necessary, clarify the component name (e.g., "Entity" becomes "Entity API" which is what we call it anyway).
Remove any totally duplicated components.

Alright.

Action
- ?

IMHO "Action Plugins" would describer things better. "Action" sounds like a movie genre. Let's add "Science fiction" to typed data.
Well in general action is a bit of a borderline candidate as it lives both as a module and a core subsystem. Maybe name it "Action UI" here?

Number
- ?

This is part of the larger fields stuff. I guess we no longer need it?

Translations
- ?

Sounds like a duplicate of

Content Translation
- Francesco Placella 'plach' https://www.drupal.org/u/plach
xjm’s picture

FileSize
21.34 KB
861 bytes

"Action" sounds like a movie genre. Let's add "Science fiction" to typed data.

This made my day.

Thanks @dawehner. I addressed all those points. For "Action" I actually just changed it to "Actions" which is the actual label of the module in the UI. It's still a bit vague but better I think.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks good for me. For anyone reviewing the patch I recommend applying the patch. This makes it much easier.

xjm’s picture

Assigned: xjm » Unassigned

Thanks @dawehner!

(This was assigned to me because I was working on it. Unassigning to make it clear I'm not the one committing it... obviously since I wrote the patch.)

daffie’s picture

I know I am a little bit late, but I like the idea of working in teams and with this patch is the Database group still divided into the different databases. Can we combine them into one group and call that group something like "Database API (MySQL, PostgreSQL and SQLite)".

xjm’s picture

Thanks @daffie!

The scope for this issue is specifically to not make any changes at all, only combine the separate sections in the file. From the summary:

Not in scope

So (as I said in #33), let's please not discuss any of that on this issue, which is only about the formatting and ordering of the current file with the current entries.

For the DB maintainer section, I think that would need to be discussed with those maintainers in a separate issue. #2660144: [Plan] Update core components is the overall issue and #2702321: Group core components into broad categories is the issue that proposes grouping core components. I will post a comment on the latter.

xjm’s picture

xjm’s picture

Issue tags: +rc eligible
daffie’s picture

+1 for updating/improving MAINTAINERS.txt.

@xjm: My apologies for my comment #38. Should have posted it on #2702321: Group core components into broad categories.

  • catch committed 2162000 on 8.3.x
    Issue #2785891 by xjm, dawehner, tim.plunkett, alexpott, catch, YesCT,...

  • catch committed a9545cf on 8.2.x
    Issue #2785891 by xjm, dawehner, tim.plunkett, alexpott, catch, YesCT,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Went ahead and committed this.

I have concerns that MAINTAINERS.txt is inherently unmaintainable (ba-doom-tish, but also not a joke). However we should exhaust trying to maintain it before doing anything more drastic, and this patch is a step forward there.

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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