Account created on 19 January 2006, about 18 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States dww
  1. Changes in the MR are small, clear, and do what the title and summary say they should. No complaints, not even nits. And I'm usually good for at least something pedantic to point out during a review. πŸ˜‚
  2. CI pipeline is all green.
  3. The fact that we might have random garbage collection during tests is definitely a source of instabilityts.
  4. This random query behavior has been causing trouble in the performance tests and other spots.

Therefore, RTBC.

Thanks!
-Derek

p.s. Out of scope, but I'll say it anyway. Expanding the full context for the MR changes, I notice this (untouched by the MR):

      // @todo Decide in https://www.drupal.org/project/drupal/issues/3395099 when/how to trigger deprecation errors or even failures for contrib modules.

But πŸ“Œ Follow-up for #3361534: config validation errors in contrib modules should not cause deprecation notices, unless they opt in Active is Closed (fixed). Do we need a followup to remove that @todo?

πŸ‡ΊπŸ‡ΈUnited States dww

Yay, thanks for the speedy review and commit now that we've reached consensus! πŸŽ‰ Extremely happy to have this bug fixed at last. πŸ™

πŸ‡ΊπŸ‡ΈUnited States dww

Thanks for resolving the thread. RTBC. Saving credits.

πŸ‡ΊπŸ‡ΈUnited States dww

P.s. we’ve already got the β€œNeeds tests” tag. Do we want to introduce something like β€œNeeds no tests” to indicate we’ve considered it and it’s not worth it, undesirable, can be punted to follow-up, etc?

πŸ‡ΊπŸ‡ΈUnited States dww

Cleaning up the summary a bit.

πŸ‡ΊπŸ‡ΈUnited States dww

+1 to #100 and #102.

Here's another to consider: what tests could / should we add for πŸ› Active toolbar tray has weak affordance and fails WCAG color criteria Needs work ? That issue has already been so painful, I don't think I could handle another 2 years for the tests. 😬 I'm not really sure what we'd be testing. It's a major accessibility bug with an entirely CSS fix. Are we supposed to have automated coverage for the accessibility of our CSS (beyond the lint job, which is already passing)?

πŸ‡ΊπŸ‡ΈUnited States dww

Yeah, you're probably right. Let's remove the tag, then. πŸ˜…

πŸ‡ΊπŸ‡ΈUnited States dww

dww β†’ changed the visibility of the branch drupal-3097907-indicate-active-tray-11x to hidden.

πŸ‡ΊπŸ‡ΈUnited States dww

dww β†’ changed the visibility of the branch 9.5.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States dww

dww β†’ changed the visibility of the branch 11.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States dww

Whoops, here's the Slack discussion.

πŸ‡ΊπŸ‡ΈUnited States dww

Per Slack thread (see attached transcript), @ckrina is now okay with the "Inverted T". I opened a new MR against 11.x with the code from patch #73.

  • Removing 'Needs accessibility review', since #73 was already approved by @andrewmacphearson, and many others as solving the original accessibility bug.
  • Removing "Needs design" since @ckrina approved this.
  • Removing "Needs issue summary update" since the summary and screenshots are now accurate again.

Ready for re-review.

Thanks,
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

Perhaps doesn't need a formal review, and will hopefully be committed before the next meeting 🀞, but if not, it'd be great to get quick confirmation that y'all are cool with the small change to the installer at ✨ Add link to Update module documentation about installer settings RTBC πŸ˜… Thanks! -Derek

πŸ‡ΊπŸ‡ΈUnited States dww

Whoops, @acbramley isn't a novice, but I think they did the work before @andypost tagged this issue as such. πŸ˜…

MR mostly looks great. Agreed with removing the line entirely in TimeInterface. Left a suggestion for a tiny nit in the Kernel test from Update module. . πŸ˜‚

Title and summary are clear. This is comment-only, so no tests needed. MR pipeline is green (as expected). IMHO, this is RTBC with my suggestion applied, and probably even without it. 😊

Thanks,
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

Thanks for the offer, @adriancid! Absolutely no offense to anyone else, but when I saw the comment, I said to myself: "Finally, a name I recognize volunteering for the task." 😊 Given your history and experience (including issues we've collaborated on before), I trust you to be a very responsible maintainer. Welcome aboard! πŸŽ‰

Because it's the only way to give you proper access to configuring GitLab for this project, I made you a "full maintainer", including the ability to change the maintainers. But please do not add additional co-maintainers without a +1 from me in an issue about it.

It'd be fabulous to resolve πŸ“Œ Setup GitLab CI for form_mode_manager Active soon, and fix any phpcs, phpstan, etc that turns up so we have clean, green GitLab CI pipelines here.

I'm trying to only commit things (both here and other projects) that have automated tests.

I personally hate the default git commit messages provided by the d.o issue UI and the conventions used for years. I've been on a campaign to try to change them. I've started experimenting with alternatives as you'll see in the git history here (and at address, pathologic, etc). Feel free to do the same, but if you default to the easy thing provided by the issue UI, I won't blame you. πŸ˜‚

Feel free to ping me anytime in Slack. I don't have email notifications enabled for this project. πŸ˜… Maybe I should. πŸ˜†

Thanks again!
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

Great, thanks! In principle, this sounds like it needs to happen and is heading in the right direction. No time for a super close review, but I see there are no new tests in the MR. I'm trying not to commit anything else here that doesn't have test coverage, tagging accordingly.

Thanks again, -Derek

πŸ‡ΊπŸ‡ΈUnited States dww

Minor summary edit, but I didn't bother with the full default template since none of the other headings are relevant here.

πŸ‡ΊπŸ‡ΈUnited States dww

Also reviewed the MR code and test results. RTBC++.

Hah, this was random #bugsmash ~1.5 years ago for patch #17, but wasn't tagged then, either. πŸ˜…

Saving credits for everyone.

πŸ‡ΊπŸ‡ΈUnited States dww

Fixed the PHPCS errors, both via phpcbf and manually.

In comment #71 @jhodgdon pointed to πŸ› Misuse of formatPlural() in Numeric field prefix/suffix Needs work where they also need the labels and decided to add something to StringTranslationTrait. That could be read as a vote for #90.2.

Also tagging that this needs tests before it could be committed, and started saving issue credits for the rich discussion here.

πŸ‡ΊπŸ‡ΈUnited States dww

p.s. re #90.2: I suggested StringTranslationTrait since it's already providing getNumberOfPlurals().

πŸ‡ΊπŸ‡ΈUnited States dww

This came up as a random triage target in #bugsmash.

I confess to not having read every comment, but this definitely still seems like a bug, and likely major is accurate.

Turned patch #30 into MR 6729

I'm sure we don't want locale_get_plural_form_labels() as a new procedural method for this as the API addition. πŸ˜‚ I'm not sure we want to try to maintain this exact list in core code like this, but I'm not yet seeing an alternative. Thankfully, once we get it right, this list will never change except when adding new languages. And multiple plural forms is pretty rare. Even in ʻŌlelo HawaiΚ»i (which I co-maintain with @xjm), a language with different versions of "we" for "the two of us" vs. "all of us", it still only has 2 plural forms for the purpose of this issue.

Where do we put the logic?

  1. Maybe this would make sense as a new public method on the locale.plural.formula service?
  2. Or add it to StringTranslationTrait?
  3. Other?
πŸ‡ΊπŸ‡ΈUnited States dww

dww β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States dww

Tagging that this came up as a random triage target today in #bugsmash, which is where the new life came from.

πŸ‡ΊπŸ‡ΈUnited States dww

This came up as a random triage target for #bugsmash. Still a bug. Nothing has changed here. The proposed resolution is still what needs to happen. But thanks, triage "bot", for reminding me this bug exists. πŸ˜‚ Tagging to be smashed, and hopefully someone wants to run with it.

πŸ‡ΊπŸ‡ΈUnited States dww

p.p.s. I'm no longer working for the client that needed this module. I don't use it on any sites. I basically don't care about this at all. πŸ˜… So perhaps anyone, regardless of their abilities / intentions, would be a better maintainer than myself. πŸ˜‚ It's funny how a sense of "responsibility" can be so selective.

πŸ‡ΊπŸ‡ΈUnited States dww

Whoops, sorry about the p.s. That was copy/paste from the reply to the other request. You did include the link in your message, thanks!

I'm not encouraging you to fork this module or otherwise add Yet-Another-Module on d.o. If you want to contribute, please do! I just don't give out push access to modules I (co)maintain without any info about the developer I'd be allowing commits from. Hope you understand and don't take it personally.

Thanks,
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

Hi @mferanda, thanks for your interest and offer to help!

However, https://www.drupal.org/project/issues/search/form_mode_manager?project_i... β†’ shows only this issue here as one that you're even following in this module's issue queue. You've never participated in any other issues, never submitted a patch, never reviewed another person's patch, etc. I can't make you a co-maintainer if I have no idea how careful you are as a coder, what your approach is, or what it's like working together. πŸ˜…

If you want to be a co-maintainer, please start meaningfully contributing to this project. Once you do, I'll get to know your abilities, style, and your ideas for the direction of the module. You can triage the queue looking for duplicates. You can review + test patches that are "RTBC" or "Needs review". You can work on things that "Need work", etc. Anything you can do to move issues forward will help me get a sense of your capabilities.

Thanks again!
-Derek

p.s. When you sent the message to me about your interest in this module via my contact tab, it would have been great had you included a link to this issue. I had to search for it to reply. ;)

πŸ‡ΊπŸ‡ΈUnited States dww

Finished addressing all threads. There's 1 I'm not sure about (see MR comments). Otherwise, this is ready for re-review.

Thanks,
-Derek

p.s. x-post with mondrake, but I noticed the same thing. That thread can also be resolved.

πŸ‡ΊπŸ‡ΈUnited States dww

- Rebased to 11.x.
- Fixed 2 of the comments from @longwave in testInfoParserBroken().
- The 4 threads about testInfoException() still need help.

πŸ‡ΊπŸ‡ΈUnited States dww

And what about the counter-proposal in #18? Maybe it's better to leave this alone, per https://en.wiktionary.org/wiki/dependee -- "Dependee" is enough of a "word" to have a wikipedia article devoted to itself. Perhaps we should call this "works as designed" and move on?

πŸ‡ΊπŸ‡ΈUnited States dww

Yeah, I’m sorry, too. I’m sorry I’ve lost my cool (at least twice) over the frustrations of trying to get this fixed through all the various roadblocks, twists and turns.

This was RTBC to fix the use of color bug years ago, and you did feel strongly that the β€œinverted T” solution using shape and color wasn’t ok from your design role, and you blocked this from going in at #69. Since then, we’ve tried (a lot) to get designs that you’re happy with that also address the bug. I’ve played operator many times, attended many meetings with the other accessibility maintainers, etc.

@saschaeggi provided a bunch of proposals in #85, which I dutifully got @andrewmacphearson to review, and I pasted his concerns in #93. Doesn’t seem those have been considered since the. We’re back to the original β€œjust blue” design that he pretty strongly rejected.

Anyway, I feel strongly that this bug has been very frustrating to fix. If you now say you don’t feel strongly about it, can we go back to the design that Andrew was certain would solve the accessibly concern, reroll #73 into an MR, and merge that? then hopefully that will at least fix this bug in 10.3 for apparently toolbar’s final release before being deprecated and replaced.

πŸ‡ΊπŸ‡ΈUnited States dww

Thanks for keeping this issue moving forward, @camilledavis!

I still wish @andrewmacphearson and @ckrina would directly discuss this issue with each other. In #93 I pasted Andrew's comments from the last time I managed to ping him in Slack about it. He was pretty clear at the time that the blue tab solution was "doubling down on colour" and not a sufficient fix for the original accessibility concern here:

https://www.w3.org/WAI/WCAG21/Understanding/use-of-color.html

Color is not used as the only visual means of conveying information, indicating an action, prompting a response, or distinguishing a visual element

#96 tried to pick up the pieces of this stalled effort and run with it. Thanks! But we've still basically only heard from the design side of this issue since then, and AFAICT, no one has confirmed if that's going to satisfy 1.4.1 which is the whole point of all the effort that we've spent trying to fix this bug. In #98 @ckrina writes "I hope this solves the accessibility concerns." but there's no explanation how the blue tab is anything other than using color alone. Yes, it's definitely higher contrast than what we have. Yes, it looks nice with the rest of the (Claro) design system. Yet it seems to be only using color to indicate what's the active tray.

I'm still pretty burned out from this whole experience, and re-reading the comment thread here isn't helping. πŸ˜… I really do not want any of us to spend more time trying to get this fixed until the two most important voices here actually agree with a single plan. Until that happens, I fear we're wasting our lives on something that will never be solved. Once again, can the two of you please talk to each other instead of having the rest of us shuttle proposals, concerns and counter-proposals back and forth?

I'm going to take the bold step of calling this "postponed" and assigning to @andrewmacphearson to get an answer so we can proceed. @ckrina, please reach out to Andrew if you have a direct channel.

Thanks so much!
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

I didn't actually test this, but the summary makes sense and the changes in the MR implement what they should. RTBC. πŸ˜…

Thanks!
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

MR had a real Kernel test failure at https://git.drupalcode.org/issue/drupal-3174819/-/jobs/829562 for the new LanguageServiceProviderTest, precisely because it wasn't setting up the language correctly. Porting it to use LanguageTestTrait and that test now passes locally.

πŸ‡ΊπŸ‡ΈUnited States dww

This came up as a random daily triage target in #bugsmash.

Looks and sounds like this is definitely still a bug. There's unaddressed feedback that needs work. The tests could be cleaned up now that we've got the new traits from πŸ“Œ Use the API to set up languages in tests that are not specifically testing the language form Fixed . The summary needs an update to clarify the proposed change.

πŸ‡ΊπŸ‡ΈUnited States dww

Maybe I'm missing something, but composer doesn't magically remove the need for tarballs. It's just a different (mostly better) way to install tarballs for you. But that's what a "dist" installation is. Composer has to get your code from somewhere. Either it's "prefer source" (Git checkout) or "prefer dist" (a "tarball").

  1. Yes, we can hide them from the UI of release nodes. πŸ“Œ Remove download of tar files Postponed Not sure why that's postponed, seems it could proceed any time.
  2. Yes, we can deprecate the parts of Update Manager that let you install "directly from tarball" via the UI once we have project browser. I believe that's πŸ“Œ Remove adding an extension via a URL Active
  3. Yes, we can deprecate the parts of Update Manager that update your currently installed codebase once we have autoupdates. Is that #3201968: Augment then Replace current Update Manager URL download based updates with Staged-Composer workflow β†’ ? The summary here lists #3417136 twice. 3201968 has its own ancestry via autoupdates, not sure we should re-parent it.

But none of that means we can "Deprecate tarballs", because all of it still depends on a dist release artifact under the hood. E.g. if you run composer require drupal/slim_select, you'll eventually see:

  - Installing drupal/slim_select (1.0.0-alpha4): Extracting archive

"Extracting archive" means tar -zxf slim_select-1.0.0-alpha4.tar.gz in this case. πŸ˜…

We can't actually do #3417136 or #3201968 (or whatever issue we should use for point 2) until we have a stable replacement. #3419966 could happen anytime.

πŸ‡ΊπŸ‡ΈUnited States dww

Moving this to the component where we need to remove things. πŸ˜…

Also, seems like this needs to be postponed on project browser in core. Ideally stable.

I'm not convinced we need to move the crappy old plumbing from 15 years ago into a separate legacy deprecated obsolete project. IMHO, we can simply remove it all once there's a better replacement built on modern foundations.

πŸ‡ΊπŸ‡ΈUnited States dww

More to the point, grepping the source tree for 'PublicStream' returns no hits with the 2.0.x branch. The only place I see that is in patches for πŸ› Internal URL handling (language prefixes, base://, ...) Needs work . So this is clearly coming from incomplete patching or something. Calling this fixed.

πŸ‡ΊπŸ‡ΈUnited States dww

Yeah, if there were a fatal error simply from installing the new version, our automated tests would have failed before I tagged the release. I think this is a support request about an incomplete upgrade to a new version. As folks have said, if you're having trouble, and a drush cr doesn't solve it, try this:

composer remove drupal/pathologic
composer require drupal/pathologic

(or equivalent).

πŸ‡ΊπŸ‡ΈUnited States dww

Indeed, I’m very familiar with that job and how it works. I’m helping to get it ported into the contrib templates. #3418831: Test-only job β†’ . That’s why I said it would not help for this issue and why I did a β€œtest-only” approach manually to make it easier for committers. πŸ‘

πŸ‡ΊπŸ‡ΈUnited States dww

GitLab CI won't really understand what "test-only" means here. πŸ˜‚ So I just applied the change to the flagWords in core/.cspell.json and ran yarn run -s spellcheck:core. The only error I got was:

./core/lib/Drupal/Core/Database/Query/SelectInterface.php:331:42 - Forbidden word (queuing)

So flagWords is working as expected. The full pipeline is green, since that is fixed here in the MR.

The summary is clear and complete. I'm updating the title to try to be more appropriate for a commit message.

The MR is all good. This is ready.

Thanks!
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

MR up with the 1-line patch that generated that screenshot. πŸ˜…

πŸ‡ΊπŸ‡ΈUnited States dww

Added the actual link from the installer as a blockquote in the 'after' section of UI changes, limit screenshot widths

πŸ‡ΊπŸ‡ΈUnited States dww

I'm all for helping people learn and understand. There's already some fine print in this part of the installer. I'd be totally in favor of making "checking for updates" a link that goes to https://www.drupal.org/docs/8/core/modules/update/overview β†’ (or whatever the "evergreen" version of that link should really be) and add a paragraph at the top of that page that talks about these two checkboxes during the installer. I would be shocked if that got rejected during usability review and I bet core committers would be fine with such a change. Technically, it would be a fairly "visible" string translation break, so it'd have to be in a whole new minor release of core for this to go out, but that's fine, since this is correctly classified as a 'feature request' already. Would that sufficiently address your concerns?

Thanks!
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

Thanks for your effort to figure out how this all works and for opening the issue. This is the first time I remember seeing anything about this. It seems like for the most part, those checkboxes made enough sense to enough people installing Drupal through the UI over the years. πŸ˜…

But it's hard to know if this is a problem that needs fixing, since we don't have a ton of usability testing and user research that shows what happened when people tried to use the installer. Maybe there's more confusion than we think, but the folks who are confused don't bother trying to report it here as a bug report, etc.

Yet I do wonder if it actually matters that much that people during their initial installation of Drupal know that 'Check for updates automatically' actually means 'The Update Manager module will be installed'. That's frankly an implementation detail of how checking for updates works. But end users don't need to know about that detail or care how it works. They just know that if they check the box, they will have a site that will 'Check for updates automatically'. It kind of seems like this ticket is being driven by a developer that wants to be clear on implementation specifics, yet those aren't the target audience for this interface. My unsubstantiated assertion is that the vast majority of people using this form either do not care, and/or do not have enough knowledge (yet) to understand how these checkboxes are going to work.

So mostly I believe this is heading in the wrong direction. Instead of asking "Can we make these checkboxes more explicitly explain how they're going to work?", we should be asking "are people not opt'ing-in to checking for updates automatically because they don't understand that's what 'Check for updates automatically' means?" Basically, instead of refining the descriptions to explain how they work, we should try to document that people are indeed failing to install Drupal how they want to as a result of these checkbox labels.

All that said, I'm not categorically opposed to adding some help text if it's needed. I just don't know it's needed, and generally the fewer words on a page, the better.

If we are going to change this, we'll eventually need a review by the #ux team ('Needs usability review'), but there's not enough of a concrete proposal yet for them to review. Tagging for 'Usability' for now.

πŸ‡ΊπŸ‡ΈUnited States dww

Definitely a big supporter of this change. We should allow multi-line here.

+1 to a newline after the initial if ( before the first condition if we're going to split it. Makes it more in alignment with our other multi-line standards, and more in line with PSR12 and the rest of the community.

Also, I strongly believe that operators at the front of the line make for more readable and understandable code. When parsing boolean logic with your brain, it seems essential to know in advance if you're building up an OR vs. AND so you can keep that in mind while reading the rest of the line. I know it's somewhat subjective, but I think there's an objective point in here that the operator is essential for understanding the rest of what you're reading. πŸ˜…

Added myself as a supporter.

I just took some of the relevant text from the PSR-12 standard here, adapted it to our NO NEED TO SHOUT conventions, and added it to the proposed changes under option A. I hope that's a friendly amendment. I believe it's useful to be explicit that if you do this, you must format it according to clear rules, not just a block of example code.

πŸ‡ΊπŸ‡ΈUnited States dww

@willeaton: You might find State Machine β†’ suitable for your needs. It provides / supports "Guards" which are plugins you can define to prevent specific state transitions if certain conditions are not met. But we're getting a bit off topic from this issue.

I also needed something like this for a project, but this issue seems a potentially doomed in deadlock between release manager and subsystem maintainer. I'm not sure I can be more persuasive than what @Sam152 already said in #40 and #45. Yes, a mix of hooks and events is somewhat crappy DX, but so is the current situation. I'm not sure how we provide 100% BC without a mix of both unless we want to make ContentModerationState public API. Drupal seems to love to provide a lot of different ways to alter the same thing πŸ˜… so I'm not totally convinced that having both Events and hooks is really all that terrible. πŸ˜‚

+1 to doing this. If I can find more time for a direct (and hopefully persuasive) response to every point in #39, I'll try.

πŸ‡ΊπŸ‡ΈUnited States dww

Agreed that this looks great. Almost nothing to complain about in the MR, title, summary, or the CR. Really excited about the issue this is blocking! πŸŽ‰

I opened a few MR threads for optional questions to address, but this is ready as-is. +1 RTBC.

Thanks!
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

+1 Yes please! If we're changing the params, we would normally have to be updating the docs to match. In this case, removing the docs is better than "fixing" them.

πŸ‡ΊπŸ‡ΈUnited States dww

+1 to "queueing" as the "correct" spelling, even though there's some indication that that's the "British" way of spelling it, while the version with 1 'e' is more "American".

British

https://www.oed.com/dictionary/queueing_adj

American

https://www.collinsdictionary.com/us/dictionary/english/queuing
https://www.merriam-webster.com/dictionary/queuing

Yet every one of those pages list both versions as correct. So it seems very subjective.

So I think the bikeshed should be painted "queueing". πŸ˜‚

Cheers,
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

We might need both a fix for the original update function, and perhaps a 2nd update function for sites that are in a partially mangled state, TBD.

I haven't yet seen this myself, so I have no insights to share from direct experience.

This seems like a blocker to a 2.0.1 release, so I'd love to get this fixed ASAP.

Thanks,
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

From triaging the queue, I'd say πŸ› Mismatched field definitions after address upgrade Needs review is probably a blocker for a 2.0.1 release. Maybe we should open a dedicated issue to plan it to make sure @bojanz is okay with it.

πŸ‡ΊπŸ‡ΈUnited States dww

Since we currently see the deprecations on DrupalCI not GitLabCI, here's a patch.

πŸ‡ΊπŸ‡ΈUnited States dww

Yeah, 3 core committers all opposed this (@quietone, @catch, @larowlan). I'm also now on the CS committee and do NOT think we SHOULD change our standards to begin SHOUTING, even though EVERYONE else does it. πŸ˜…

The CS process has changed a lot over the years since this issue was opened. If anyone strongly wants to revisit this, please open a new issue about it and try to get new supporters, but given how many of the current committers and committee members oppose it, I doubt it's going to happen.

Thanks,
-Derek

πŸ‡ΊπŸ‡ΈUnited States dww

Would love another formal agenda item about "Could we consider the Git history part of the "code" and spend some energy on improving the format of our commit messages?"
🌱 Discuss if git commit messages should be multiple lines Needs review
🌱 [policy, no patch] Determine format for commit credit for individuals/organizations/customers Needs review
https://www.conventionalcommits.org/en/v1.0.0
...

πŸ‡ΊπŸ‡ΈUnited States dww

Cool, updated based on all the improvements discussed so far: https://www.drupal.org/node/2465321/revisions/view/13405201/13405210 β†’

Then, re-reading it, I realized the previous step about core not affected was wrong:

  • If Core is not affected by the change the issue is tagged β€œNeeds documentation updates”.
  • That's not true. We would still have to wait for public feedback, even if core is not affected. We'll only add 'Needs documentation updates' in our final review. There's actually nothing special to do in this case, so I removed the "step" entirely: https://www.drupal.org/node/2465321/revisions/view/13405210/13405214 β†’

    Also updated the default issue summary template:

    • Removed the same bogus step for the non-core case.
    • Added 'Final review by Coding Standards committee'

    https://www.drupal.org/node/2465321/revisions/view/13405214/13405215 β†’ (whoops, no "visible" changes). Here's the new full text:

    Problem/Motivation

    State what you believe is wrong or missing from the current standards.

    Benefits

    If we adopted this change, the Drupal Project would benefit by ...

    Three supporters required

    1. https://www.drupal.org/u/ β†’ {userid} (date that user added support)
    2. https://www.drupal.org/u/ β†’ {userid} (date that user added support)
    3. https://www.drupal.org/u/ β†’ {userid} (date that user added support)

    Proposed changes

    Provide all proposed changes to the Drupal Coding standards β†’ . Give a link to each section that will be changed, and show the current text and proposed text as in the following layout:

    1. {link to the documentation heading that is to change}

    Add current text in blockquotes

    Add proposed text in blockquotes

    2. Repeat the above for each page or sub-page that needs to be changed.

    Remaining tasks

    1. Add supporters
    2. Create a Change Record
    3. Review by the Coding Standards Committee
    4. Coding Standards Committee takes action as required
    5. Discussed by the Core Committer Committee, if it impacts Drupal Core
    6. Final review by Coding Standards Committee
    7. Documentation updates
      1. Edit all pages
      2. Publish change record
      3. Remove 'Needs documentation edits' tag
    8. If applicable, create follow-up issues for PHPCS rules/sniffs changes

    For a full explanation of these steps see the Coding Standards project page β†’

    πŸ‡ΊπŸ‡ΈUnited States dww

    Updated summary again to incorporate all my proposals:

    • #10.1: Confirmed
    • #10.4: Remove the sentence and link to TWG charter
    • #11.1: Move / re-order
    • #11.2: Use 'Committee' (capital C) in h3
    πŸ‡ΊπŸ‡ΈUnited States dww

    Yay, πŸ› Field type plugin description is assumed to be an array Fixed is now fixed. Not that it needed to block this, but it's nice that it's done. This is clearly working, and the fact that the D10 tests are failing in DrupalCI is annoying and misleading. Committed to 2.x and cherry picked to 2.0.x and 8.x-1.x.

    Thanks!
    -Derek

    πŸ‡ΊπŸ‡ΈUnited States dww

    Thanks! All the getEntityLabels() cleanup stuff already have dedicated issues. We don't want to change that here, nor open any new issues:

    πŸ“Œ Remove hardcoded word 'entities' in EntityInlineForm::getEntityTypeLabels() Fixed
    πŸ“Œ InlineEntityFormBase::getEntityTypeLabels() has the wrong docblock Needs review
    πŸ“Œ Deprecate InlineFormInterface::getEntityTypeLabels() Needs review

    That said, I'm kind of torn here. If we're supporting D9 (+1 to that), seems weird to require a higher PHP than core did for 9.x. I'm reluctant to move forward with dropping PHP 7.4 support until we're ready to drop D9, as nice as that would be for us maintainers...

    πŸ‡ΊπŸ‡ΈUnited States dww

    Yes, please. Thanks!

    πŸ‡ΊπŸ‡ΈUnited States dww

    Pipelines are consistently happy again with this change, so merged, cherry-picked and pushed.

    πŸ‡ΊπŸ‡ΈUnited States dww

    Okay, here's a real failure from a job with CI_DEBUG_SERVICES enabled πŸŽ‰

    https://git.drupalcode.org/project/address/-/jobs/752891

    Logs are full of this:

    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:53.851198940Z netcat: connect to localhost (127.0.0.1) port 3306 (tcp) failed: Connection refused
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:53.851225416Z netcat: connect to localhost (::1) port 3306 (tcp) failed: Cannot assign requested address
    

    Here's the real culprit:

    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451301665Z 2024-02-05T21:59:23.451125Z 0 [ERROR] InnoDB: io_setup() failed with EAGAIN after 5 attempts.
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451322612Z 2024-02-05T21:59:23.451154Z 0 [Note] InnoDB: You can disable Linux Native AIO by setting innodb_use_native_aio = 0 in my.cnf
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451325887Z 2024-02-05T21:59:23.451266Z 0 [ERROR] InnoDB: Cannot initialize AIO sub-system
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451328497Z 2024-02-05T21:59:23.451274Z 0 [ERROR] InnoDB: Plugin initialization aborted with error Generic error
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451331115Z 2024-02-05T21:59:23.451281Z 0 [ERROR] Plugin 'InnoDB' init function returned error.
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451333559Z 2024-02-05T21:59:23.451285Z 0 [ERROR] Plugin 'InnoDB' registration as a STORAGE ENGINE failed.
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451338953Z 2024-02-05T21:59:23.451289Z 0 [ERROR] Failed to initialize builtin plugins.
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451341452Z 2024-02-05T21:59:23.451292Z 0 [ERROR] Aborting
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451344050Z 
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451346775Z 2024-02-05T21:59:23.451304Z 0 [Note] Binlog end
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.451465428Z 2024-02-05T21:59:23.451358Z 0 [Note] Shutting down plugin 'CSV'
    [service:drupalci/mysql-5.7-database] 2024-02-05T21:59:23.453778406Z 2024-02-05T21:59:23.453665Z 0 [Note] /usr/sbin/mysqld: Shutdown complete
    

    However, it's not clear why that is happening, just from these logs. Wonder if there's other output being saved somewhere that might be useful. Hopefully @fjgarlin has a chance to review this and knows where to look for the underlying problem.

    πŸ‡ΊπŸ‡ΈUnited States dww

    Just got hit by this again:

    https://git.drupalcode.org/project/address/-/pipelines/88194
    https://git.drupalcode.org/project/address/-/pipelines/88195
    https://git.drupalcode.org/project/address/-/pipelines/88196

    But then I remembered I needed to turn on this extra variable, pushed that to our 2.x branch, and now the pipeline is failing in a totally different, unrelated way:

    https://git.drupalcode.org/project/address/-/pipelines/88205

    Instead of failing to connect to the DB, it's now complaining about 5 cases of this:

    1) Drupal\Tests\address\FunctionalJavascript\AddressDefaultWidgetTest::testCountries
    TypeError: Behat\Mink\Element\TraversableElement::findButton(): Argument #1 ($locator) must be of type string, Drupal\Core\StringTranslation\TranslatableMarkup given, called in /builds/project/address/web/core/tests/Drupal/Tests/WebAssert.php on line 143
    /builds/project/address/vendor/behat/mink/src/Element/TraversableElement.php:97
    /builds/project/address/web/core/tests/Drupal/Tests/WebAssert.php:143
    /builds/project/address/web/core/tests/Drupal/Tests/UiHelperTrait.php:78
    /builds/project/address/tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php:193
    /builds/project/address/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    

    None of that fails locally, and the previous commit I pushed had nothing to do with the tests. I have no idea why these are failing like this now. πŸ˜… But we don't want t() in those tests, anyway, so I opened πŸ“Œ Remove t() from tests that are not testing translation Fixed to clean that out. Hopefully the pipelines on that MR are now green. TBD.

    πŸ‡ΊπŸ‡ΈUnited States dww

    That's hilarious - I thought we had the "all core components are reserved" thing in place for years. πŸ˜‚ Whoops. Glad it's locked down now.

    Thanks!
    -Derek

    πŸ‡ΊπŸ‡ΈUnited States dww

    I committed this to the address repo. Thanks, everyone!

    πŸ‡ΊπŸ‡ΈUnited States dww

    Went with the borderless version. Also updated project page. Thanks, everyone!

    πŸ‡ΊπŸ‡ΈUnited States dww

    Oh whoops, I don't have perms to save credits in this queue. @quietone, would you be willing to credit all of us?

    πŸ‡ΊπŸ‡ΈUnited States dww

    Great, thanks! Instead of letting perfect be the enemy of good, since there are at least 2 of us in agreement that 'Favorite Primary Color' is better, let's go with that. It's only a stale handbook page, after all. πŸ˜… Let's fix the bug and move on.

    https://www.drupal.org/node/2192175/revisions/view/13394743/13404985 β†’

    Thanks again,
    -Derek

    πŸ‡ΊπŸ‡ΈUnited States dww

    Fix bug #3411977: Gender binary example on (2192175) "Creating a content entity type in Drupal 8" β†’ : Replace the 'Gender' (binary) field with a 'Favorite Primary Color'

    πŸ‡ΊπŸ‡ΈUnited States dww

    Also crediting @kingdutch for the MR and myself for reviews, followup, etc.

    πŸ‡ΊπŸ‡ΈUnited States dww

    Oh, and I guess that means I can RTBC this, since I didn't touch the code. Thanks again!

    πŸ‡ΊπŸ‡ΈUnited States dww

    If y'all are willing to do this manually, there are some advantages. Mainly, we wouldn't be adding one more thing that can go wrong to the mix. πŸ˜‚

    Glad the check_versions job is now correctly configured, thanks!

    Copying myself from Slack:

    the bummer is that a ton of contrib projects out there now have their own copy of the template that started with this:

    ################
    # Pipeline configuration variables
    #
    # These are the variables provided to the Run Pipeline form that a user may want to override.
    #
    # Docs at https://git.drupalcode.org/project/gitlab_templates/-/blob/1.0.x/includes/include.drupalci.variables.yml
    ################
    

    All of those copies now have to be "fixed" to point to the correct branch. That's where the confusion came from. I'm following this whole thing somewhat closely and I had no idea that 1.0.x was now dead. 98% of contrib maintainers won't know the docs links in their templates are actually wrong.

    Then @fjgarlan replied:

    As that’s the only obsolete link in the template that a lot of people have I could make an extra commit to that file (on the old branch) to show that it’s deprecated and include a link to the right one.

    Not perfect but it could help in these cases.

    So yeah, let's close this as "works as designed".

    Thanks,
    -Derek

    πŸ‡ΊπŸ‡ΈUnited States dww

    Okay, the 10.2.x MR should be happy now:

    Full pipeline: https://git.drupalcode.org/issue/drupal-3415412/-/pipelines/86412 (all pass)
    Test-only job: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/733522 (failed as expected)

    In Slack, @larowlan told me to self-RTBC, so here goes.

    πŸ‡ΊπŸ‡ΈUnited States dww

    Was pointed here from Slack. I absolutely love the test-only jobs in the core pipelines! πŸŽ‰πŸ˜ Would love to have those available in contrib, too. Thanks!

    πŸ‡ΊπŸ‡ΈUnited States dww

    Sorry about that. Fixed in the 11.x MR.

    Full pipeline: https://git.drupalcode.org/issue/drupal-3415412/-/pipelines/86386 (all passed)
    Test-only job: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/733182 (failed as expected)

    Also, since this doesn't cleanly cherry pick to 10.2.x, opened a separate MR for that:
    https://git.drupalcode.org/project/drupal/-/merge_requests/6424
    Full pipeline: https://git.drupalcode.org/issue/drupal-3415412/-/pipelines/86392 (super weird, failed with a phpcs violation that I didn't touch with the changes in the branch).
    Test-only job: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/733251 (failed as expected)

    πŸ‡ΊπŸ‡ΈUnited States dww

    Here's the test-only job: https://git.drupalcode.org/issue/drupal-3415412/-/jobs/732794

    There was 1 error:
    1) Drupal\Tests\field_ui\Functional\ManageFieldsTest::testAddField
    Behat\Mink\Exception\ResponseTextException: The text "This one-line field description is important for testing" was not found anywhere in the text of the current page.
    /builds/issue/drupal-3415412/vendor/behat/mink/src/WebAssert.php:811
    /builds/issue/drupal-3415412/vendor/behat/mink/src/WebAssert.php:262
    /builds/issue/drupal-3415412/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php:145
    /builds/issue/drupal-3415412/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    ERRORS!
    Tests: 9, Assertions: 157, Errors: 1.
    

    πŸŽ‰

    Production build https://api.contrib.social 0.61.6-2-g546bc20