Hello, everyone. Welcome to the :sparkle: CLI in Core Initiative :sparkle: Meeting! :calendar: This meeting:
➤ Usually happens weekly either at Wednesday 14:00 UTC or Wednesday 22:00 UTC.
➤ Will start synchronously for 30 minutes with an open agenda.
➤ Will then switch to threads to continue discussions.
➤ Please comment on the roll call thread throughout the day.

Note: this meeting’s discussions extended over several days.

0️⃣ Roll Call! Who is here today? Introduce yourself including your name and drupal.org username. If you feel comfortable, tell us something interesting about Athens or Greece.

mradcliffe Synchronous meeting attendees: larowlan, rymo, mradcliffe, tekNorah
mradcliffe Hi, Matthew (mradcliffe). I find puns interesting so I like to think that the reason ancient Sparta rose to power was because they had iron and their clothes were so neat.
mott Ryan Mott u/rymo :wave::skin-tone-2:
dww :wave: dww living in the Hawaiian Kingdom. Not in Greece this week (I wish!).
bircher bircher I am in greece and I guess I would have been awake for the call but I guess not in the mental state

1️⃣ CLI in Core Meta Issue—https://www.drupal.org/node/3582246 Reminder if you want to be pinged (or not) for one of the synchronous meetings, add your Slack username to the issue.

1️⃣ 0️⃣ Meeting notes for 2026.04.16 meeting need to be created.

mradcliffe I'll get to this tomorrow probably.

1️⃣ 1️⃣ Added translation as Phase 4.

2️⃣ CLI entry point in Drupal Core (dr)#3453474: CLI entry point in Drupal Core
2️⃣ 0️⃣ Current Status:

mradcliffe We were close to Needs review / RTBC, but back to Needs work because of the cache:rebuild issue (refer to thread below). The symfony/runtime issue is RTBC and passing. The naming issue had some good discussion/bike shedding about names and there is new consensus around "dr" rather than "pal" or "dex" (refer to thread below). Testing instructions for both core checkout and project template were added.
mradcliffe For anyone not aware regarding the process for adding the symfony/runtime dependency,#3552922: Add symfony/runtime dependency to core#comment-16560695, which essentially will be that will be merged whenever one of the issues proposing usages of it is also RTBC.

2️⃣ 1️⃣ app root issue with cache:rebuild not working on composer projects.

mradcliffe - cache:rebuild complexity due to procedural function and not finding the correct app root.- In the synchronous meeting we reviewed the spreadsheet of commands to talk about replacing with core:status (too verbose of output) or core:cron (simple output and implementation, test module in core to help with writing tests).
mradcliffe I tried out implementing cron, and it works rather well. Bikeshedding. I think it should go in system module because that is where CronController lives, but the CronInterface lives in Drupal Core.
mradcliffe What does everyone think about making 1 command for phase 1 cron instead of cache:rebuild due to complexity? And that would give time for people work on other issues trying to figure out app root changes to do so.
mradcliffe I think I have time to continue working on cron, and might push that up in addition to cache:rebuild if there is not a consensus about removing it in the next day or so.
mradcliffe I'll see how far I get and then someone can hack on it at Dev Days tomorrow.
mradcliffe Reading through CronRunTest, there is not example of testing the false return value from the cron service. But it's probably in a kernel test somewhere. (edited)
mradcliffe i believe i am too tired to do anymore judging by my lack of quality. Tomorrow is another day.
dww CronCommand looks good. I pushed some cosmetic cleanups (both to that, and the rest of the MR). Also rebased on latest main while I was at it.
dww Not sure about removing cache:rebuild at this point. Would be lovely to get it working instead of remove. But let's see what happens...
mradcliffe I think we wanted 1 command as an example so we don't make the MR too big.
mradcliffe When you get back, @moshe, what do you think about removing CacheRebuildCommand for CronCommand due to the app root complexities and solving that later in Phase 2?
mradcliffe https://drupal.slack.com/archives/C079NQPQUEN/p1776849488103579
mradcliffe 2️⃣ 2️⃣ CLI name discussion (deprecate drupal)
mradcliffe @mott pushed changes to deprecate drupal for dr based on the discussion in the naming issue #3583795: [policy, no patch] Decide naming the new CLI entry point to dr)
dww So can we close the naming issue now?
mradcliffe I think we should re-open as Needs review actualyl.
mradcliffe I wasn't sure if there was enough time given for people to talk about dr or not.
mott well here's a data point for zsh users: v/b/d tab-completes nicely to either dr or drupal but v/b/p gets you a list of 8 hits to wade through

2️⃣ 3️⃣ New merge request review comments

dww Lots of 'em. :sweat_smile:
mradcliffe Thanks to @xjm and @Aaron McHale for providing review comments with suggestions. There are contribution opportunities here. Including but not limited to:- There are some suggestions for the CLI documentation @Jack Franks (he/him) last worked on.
dww On a quick skim, I think the vast bulk of @xjm's suggestions should be applied. Shall I take a first pass at doing a more thorough review/apply cycle on that? Is that helpful? Or does someone else want to do so?
mradcliffe Feel free. I think that would be helpful. Maybe not the CacheRebuildCommand/Test related changes as we might want to pull that command from the merge request due to complexity.
dww Okay, applied most of the suggestions. Resolved a few more of the threads by fixing what was requested (e.g. s//`/ etc). Back down to 9 open threads. :sweat_smile:
dww (Moved other comments here to new threads. See 2️⃣ .4️⃣ and 2️⃣ .5️⃣
dww Down to 12 open threads:6 of them are from me for 2️⃣.4️⃣ and the debate around final.4 of the others are requests for comment clarifications from xjm (which I'm not personally comfortable enough resolving, since I think she's right that the existing comments don't make much sense and I don't yet understand enough to improve them).A discussion around core/lib/Drupal/Core/Command/ConsoleCommandDiscovery.php and if the 1 static helper method in there should move directly into core/lib/Drupal/Core/Command/DrupalApplication.php The problem of 2️⃣.1️⃣ and cache:rebuild not actually working.
mradcliffe I tried to clarify some of the comments. I wasn't able to dig into ConsoleTest enough to figure out where TestApplication -> Application sets an error handler. It does set an exception handler. I couldn’t find anything in symfony calling set_error_handler when I added that as a breakpoint (only phpunit and drupal tests and kernel).
mradcliffe I copied the static method into a protected method.
mradcliffe I continued digging deeper into ConsoleTest and rewrote the comment regarding restore_error_handler , which I think sufficiently explains the usage.
mradcliffe I am not sure if the other grammar fixes I did to ConsoleTest are enough to explain that comment though - testConsoleCommand.
mradcliffe If those are okay in review, then that leaves cache rebuild.

2️⃣.4️⃣ Should core commands be final? I don't think so. See MR threads for details, or discuss here.

dww Not to rehash all of #3488476: [policy, no patch] Allow using @final when a class is not considered as an extension point again, but I don't see what we gain, and I imagine what we lose, by marking core commands as final. Can't we let contrib and custom code extend / expand core commands, give them another name with #AsCommand but inherit much of the plumbing? Why lock them down?
mradcliffe I think the commands should be final, but we should have a Base class or Trait that comes out of creating commands in Phase 2.
mradcliffe I have gotten bitten by non-marked as internal before so it is nice to know what is public API and what is not from my IDE.
mradcliffe But at the same time there should be a public API for commands.
dww Thinking down the road a bit. Let's say cache:rebuild works. And I have a setup where I want to clear some other external cache anytime it happens. I don't want to copy-paste the whole thing. I want to subclass it, and extend the logic to call out to my other (non-Drupal) service.
dww I don't want a base class or a trait. some commands are so trivial, what does that help? I want to be able to extend logic in classes.
dww What’s the point of OOifying our code and then preventing people from using OO as intended?
bircher shouldn't that be an event "hook" that runs after or beforehand etc to modify the commands?
dww P.s. there's already a base class, right? Command
dww We just don't own it.
dww I'm not sure how a hook would solve my concern. What if I need to selectively turn off something the command is doing? Or whatever. I'm somewhat making thing up, but the point is how can we possibly know what everyone else needs or wants.I still fail to grasp why “everyone” is so excited about preventing OO from working.
mradcliffe I think it's to allow us to make breaking changes. 🙂
dww Then use @final and/or @internal. if folks want to “break glass and do it anyway” it's on them. But final completely prevents people from using OO.
mradcliffe I caught up on a year worth of discussion. Agreed. I think the only definite thing we can do is remove final and add @internal because the commandline tools are still considered internal (for now).
mradcliffe and we don't have a @final policy yet.
mradcliffe Oh, great, ContentExportCommand is already final
nicxvan We can still use @final even without a policy
nicxvan It just doesn't carry as much weight
nicxvan @internal is redundant since a you pointed out commands are already internal
nicxvan But it doesn't hurt
mradcliffe There's an issue to add @internal to all internal classes and traits 🙂
dww As I said, some of the existing commands are final because they're in “experimental” parts of core. And someone else thought it was a good idea to mark “in progress” as “final”. But I'll bet you good money those same folks will quietly forget to unfinalize their stuff when they're ready to call it “stable”. I'm not going to die on this hill trying to undo that mess. But the new commands we're adding here, IMHO, should not be final. Period.
mradcliffe I removed final from the commands and the compiler pass.
nicxvan Thanks!
mradcliffe On the other hand i guess the final on the experimental commands meant it was okay to move them into a different directory :smile:

2️⃣.5️⃣ What should we return inside commands? There are currently 3 different ways to get SUCCESS. We should pick one and standardize.

dww The MR is currently returning each of these somewhere:Command::SUCCESSself::SUCCESSstatic::SUCCESS
dww I don't care which one we use, so long as we're consistent. I guess if you want to vote here, you can 1️⃣ , 2️⃣ or 3️⃣ the above comment.
dww Note that test code is asserting with Command::SUCCESS (since it has to, since the test isn't a command). So maybe we should use Command::SUCCESS everywhere, both inside commands and outside.
dww Meanwhile, core is pretty inconsistent about self:: vs. static:: . Super rough grep:% egrep -ir 'self::' core/lib/Drupal | wc
554 3470 64810
% egrep -ir 'static::' core/lib/Drupal | wc
782 4161 92001 ¯\_(ツ)_/¯
longwave self and static mean slightly different things in terms of inheritance which is why they might both be used (though in many cases they end up the same result)
mradcliffe I tend to stick with self but if the classes are final then it doesn’t really matter.
mradcliffe If commands are not final, then it should not be self because I think that means a command could change the const value to something non-standard.
dww I think final is a red herring here. Whether or not we allow additional extending, we could in theory override the parent’s constants in our own commands. If we care about that, we should probably use Command::SUCCESS everywhere, no? That also give consistency with non-command usages like tests.
longwave it is more immediately readable as to what it means
dww Okay, pushed https://git.drupalcode.org/project/drupal/-/merge_requests/14902/diffs?commit_id=9daf2f37d924a4b9821a7bea067b83c01bfaac72 to standardize on Command::

Participants:

larowlan, tekNorah, rymo, mradcliffe, dww, bircher, nicxvan, longwave

Comments

mradcliffe created an issue. See original summary.

smustgrave’s picture

smustgrave commented
Status: Needs review » Fixed

Assigned credit to the users mentioned above.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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