Drupal 10 readiness meeting / 14 February 2022 [#3259755]

Meeting will happen in #d10readiness on drupal.slack.com.

Hello and welcome to this Drupal 10 readiness meeting!

This meeting:
➤ Is for core and contributed project developers as well as people who have integrations and services related to core. Site developers who want to stay in the know to keep up-to-date for the easiest Drupal 10 upgrade of their sites are also welcome.
➤ Usually happens every other Monday at 19:00 UTC.
➤ Is done over chat.
➤ Happens in threads, which you can follow to be notified of new replies even if you don’t comment in the thread. You may also join the meeting later and participate asynchronously!
➤ Has a public agenda anyone can add to: `https://www.drupal.org/project/drupal/issues/3259755`
➤ *Transcript will be exported and posted* to the agenda issue. For anonymous comments, start with a :bust_in_silhouette: emoji. To take a comment or thread off the record, start with a :no_entry_sign: emoji.

0️⃣ Who is here today? Comment in the thread below to introduce yourself.

shaal	Ofer Shaal, DrupalPod, Umami :blob_wave:
Björn Brala (bbrala)	Ah, might as well then, Björn, JSON:API and workhorse lol ^^
xjm	:wave: xjm, core release manager
Gábor Hojtsy (he/him)	hi all! Gábor, Drupal 10 coordinator (edited)
Brad Jones	Brad Jones, lurking
Spokje	Frank, current moving-core-tests-around-specialist (mostly due to the lack of actual experts)
Björn Brala (bbrala)	hahaha
mallezie	Tim Mallezie, mostly following phpstan related things
catch	Nat, late.
larowlan	lee :wave:
Ilcho Vuchkov (vuil)	Ilcho Vuchkov, Bulgaria EU
daffie	Hi everybody
quietone	Hi, catching up
Kristen Pol (she/her)	Kristen, California, catching up

1️⃣ Do you have suggested topics you are looking to discuss? Post in this thread and we’ll open threads for them as appropriate.

shaal	DrupalPod now supports Drupal10 (for core issues).I'd love to get feedback if anything is not working as it should.
Björn Brala (bbrala)	Mostly an update; Getting HAL out of core is being worked on, but there is a few steps involved. We are getting help, but the current step (moving tests) is quite the MR to review :x - #3263654: Move all HAL tests to the module in preparation of removal in D10
xjm	The proposed change to the process for adding rector/upgrade status conversions (both the intermediate proposal of filing issues in the rector queue for triage when we remove deprecated code with CR links, and the longterm proposal of making a change to the change record node bundle's fields. Aside: It took me a second to remember the word "bundle" because it's been so long since I talked about them. :joy:
xjm	#3257127: Trigger a deprecation message when a deprecated module or theme is enabled also just needs a reroll plus some small changes.
Björn Brala (bbrala)	Thats on my todo list @xjm
Björn Brala (bbrala)	:slightly_smiling_face:
mallezie	Anyone has remarks / improvements / problems encountered now phpstan is added to core development? Love to hear feedback / suggestions / next steps...(We'll probably have work till D14 to get it clean on level 9)
xjm	Also: Should the module "removal" issues against 10.x actually be converting them to empty "outdated" stubs like we did in D9? And also, regardless, the "removal" issues will need manual testing that the core/contrib move is smooth for sites with the module enabled.

2️⃣ DrupalPod now supports Drupal 10! Feedback welcome!

Gábor Hojtsy (he/him)	Suggested by @shaal
Kristen Pol (she/her)	@shaal This is amazing! I tried installing 2 days ago in Chrome by removing my extension and installing again but it didn't work. Any advice?
shaal	Oh. What didn't work? There's nothing to install yet. The browser extension is the same, but the backend of Drupalpod has all these improvements.
Kristen Pol (she/her)	Hmm… I didn't see D10 or 9.4 in the list so maybe I'm not understanding the improvement:)
shaal	@Kristen Pol (she/her) I wrote it in my long #drupalpod updateWhen working on core issues, it will read the Drupal core version from the issue itself and will ignore the dropdown options. UI will be fixed in the upcoming new browser extension upgrade.So the browser extension look the same. But when working on core issues, the DrupalPod backend knows how to read the real version from the Drupal core issue itself
Kristen Pol (she/her)	Oh! I read that before but it didn't really register... okay, thanks for the clarification :slightly_smiling_face:

3️⃣ Congrats on removing deprecated code doing well :slightly_smiling_face:

Gábor Hojtsy (he/him)	Even the Drupal multilingual migration (placeholder) module got removed. :slightly_smiling_face: (edited)
Spokje	I think that's the first module that actually is commited and thus removed from D10?
Gábor Hojtsy (he/him)	Hm, possible. @catch @xjm any contradicting recollection? :slightly_smiling_face:
xjm	Technically, yes, because it was already deprecated in D8/9
xjm	Actually, come to think of it, all our module "removals" in core should be converting them to stubs to avoid fatal errors on upgrade
xjm	And then we remove the stubs in the next major
Spokje	I'll take your technically yes and mark that as a victory.
xjm	(That might actually merit a thread re: "outdated" stubs, needs testing at least) (edited)
Spokje	Actually, come to think of it, all our module "removals" in core should be converting them to stubs to avoid fatal errors on upgradeI'm pretty sure either @catch or @alexpott stated in some deprecation/removal issue that a full nuke was the way to go, I'll do some queue archaeology on that and report back
xjm	@Spokje Well, it is a release management decision, ultimately (so not up to alexpott). I am pretty sure it was @catch’s suggestion to put in the empty D9 stubs in the first place.
xjm	For this same reason
xjm	Let's discuss in its own thread :slightly_smiling_face:
Gábor Hojtsy (he/him)	Multilingual Drupal Migrate was a stub already.
xjm	Right, so was entity reference. But simpletest was a proper module that got converted to a stub, same with Place Block I think.
xjm	In its own thread :slightly_smiling_face:
andypost	thank you all, I think next for removal is entity_reference #3188858: Remove the entity_reference module entirely on the Drupal 10 branch (edited)

4️⃣ Forum module is being removed and needs help with core patch

Gábor Hojtsy (he/him)	@larowlan suggested this outside of the meeting, but bringing it here
Gábor Hojtsy (he/him)	#3261653: Remove Forum module needs help with moving the tests around
Björn Brala (bbrala)	Yeah i noticed that, i'm not sure i am into doing another run of many tests lol, although forum might be less integrated than hall (edited)
xjm	I already did this once for QuickEdit myself, so we can pass the joy onto someone else too :wink:
Gábor Hojtsy (he/him)	hah :smile: also Forum would need its own issue/patch/MR for this not directly in the MR removing the module
Björn Brala (bbrala)	Yeah it will, like we did for HAL (as @larowlan mentioned ;))
xjm	Yep, +1 for filing a separate blocking issue
Gábor Hojtsy (he/him)	commented there
Björn Brala (bbrala)	I'll do the administration on that. (and opening my editor to do a quick search for forum in core ;))
Björn Brala (bbrala)	oh ok
Gábor Hojtsy (he/him)	@Björn Brala (bbrala) no please do take it on :slightly_smiling_face: I just commented that this is how the process works
Björn Brala (bbrala)	hehe
Björn Brala (bbrala)	I'm just gonna make the issue, today and tomorrow i'm in the middle on an ISO audit, really need my brain for that :stuck_out_tongue:
Björn Brala (bbrala)	phpstorm wont load. Lovely.
Björn Brala (bbrala)	the world is trying to tell me something i geuss.
Spokje	If I get through the current expedition through aggregator -land here: #3264122: Move all non migration aggregator tests to the module in preparation of removal in d10 and get that one to Needs Review this would be the next on my radar.
Björn Brala (bbrala)	nice, (i've fixed a small copy paste error in the is ;))
Kristen Pol (she/her)	This ticket was created a couple days ago… was out of commission yesterday so sorry for the delay[#3264195]
Björn Brala (bbrala)	Hmm, I mughtve openened a second one then?
Kristen Pol (she/her)	Maybe it can be repurposed for another forum task? @larowlan ^ ? (edited)
larowlan	we need one to handle the migrations, see comments from @quietone
Kristen Pol (she/her)	@Björn Brala (bbrala) Would you please update your issue for that ^ Thanks:)
Björn Brala (bbrala)	@Kristen Pol (she/her) i ended up closing the one i openened since is was duplicated by yours, and the ID of yours (well, actually made by @joshua1234511) was lower ^^Yours: #3264347: Move all forum tests to the module in preparation of removal in Drupal 10 (edited)
Kristen Pol (she/her)	Thanks for the update :+1:

5️⃣ HAL module is being removed and needs review

Gábor Hojtsy (he/him)	@Björn Brala (bbrala) suggested this
Gábor Hojtsy (he/him)	#3263654: Move all HAL tests to the module in preparation of removal in D10
Spokje	We already have to Eagle Eyes and Big Brains of @daffie and (soon) @longwave on reviewing on this biggie, but any help is welcome.
daffie	I have reviewed and this should be ready to commit very soon.
Spokje	Thanks again @daffie !I answered your question in the issue, could you have a look and comment if it makes sense/actully answers your question?
daffie	It is now RTBC.

6️⃣ Rector issues for deprecations so we can make sure contrib is well taken care of

Gábor Hojtsy (he/him)	suggested by @xjm
xjm	So we discussed this a couple times in Slack over the past few days. The current proposal is: when an issue removing deprecated code is committed, file issues in the Rector queue https://www.drupal.org/project/rector for subject matter experts to triage as to whether it needs phpstan or not, Upgrade Status or not, etc.
xjm	It'd be good if we could start doing this at RTBC instead of on commit -- basically, look for CRs in the patch, and make sure issues are filed for them
xjm	Then, in the long term, one proposal would be to make this a field on the change record (at the top, I'd say) for "Needs deprecated code conversion issue" (or something), maybe checked by default (but uncheck if it's not a deprecation CR), and exposing that on the view, so that folks could get a listing of issues without it needing to churn through several steps of the core issue queue nor be mixed in with all the CRs.
mglaman	@Gábor Hojtsy (he/him) I wonder your thoughts on this. Seems like all change records of this sort impact Rector rules. But only some may require work in upgrade_status or phpstan-drupal. So best to go into Rector
mglaman	Seems the best, given we can’t have a series of checklists for the CR to say “coverage of changes based on these tools”
Gábor Hojtsy (he/him)	@mglaman yeah I think rector is a fine place, but they also need to be triaged for impact which is where the deeply deprecation checking helps
Gábor Hojtsy (he/him)	I mean if there are people standing by with a deep desire to write rector rules, nobody should stop them :smile: but if there is limited availability to cover them, then we should be sure not to take them in order of time but in order of magnitude :slightly_smiling_face:
mglaman	Well, as it stands, no on else is writing them
mglaman	My main concern is just that these CRs get created and we play catch up at the tail end, instead of having a backlog. So getting a backlog started helps
Gábor Hojtsy (he/him)	yup, so I think its still best to prioritise by impact once the deprecation checking data is available from contrib
Gábor Hojtsy (he/him)	before jumping to conclusions >D
Gábor Hojtsy (he/him)	but having the issues is super helpful for various reasons, eg. phpstan-drupal coverage too
mglaman	I see what you mean, less about following CR but actually what gets reported
Gábor Hojtsy (he/him)	(which is somewhat of a chicken and egg, since the deprecation report will not contain them if they are not found by phpstan-drupal)
mglaman	Correct
mglaman	That’s where I wish we could have the draft PR and say:Code change, requires rectorOh, and we can’t detect it via phpstan-drupal either, so it needs an enhancementwhere more often than not it’s just the rector work

7️⃣ Raising deprecation message when a deprecated module/theme is enabled

Gábor Hojtsy (he/him)	This is REALLY close, needs a reroll #3257127: Trigger a deprecation message when a deprecated module or theme is enabled
Gábor Hojtsy (he/him)	raised by @xjm
Björn Brala (bbrala)	On my todo list but probably cant work on it until friday.
xjm	Anyone else interested? This is a nice novice task, even
xjm	Reroll & address one small formatting cleanup
Björn Brala (bbrala)	Yeah, should be quite easy.
Spokje	Just adopted that issue to do a reroll and fix-before-fix-on-commit...

8️⃣ Now that phpstan is used for more than deprecation checking in core, what do people think about it? Feedback/remarks/suggestions/next steps?

Gábor Hojtsy (he/him)	raised by @mallezie
andypost	as CR said it usable only for core https://www.drupal.org/node/3258232there's extra work on deprecation removals to check exceptions to clean-up
andypost	I think next step is allow use it for contrib modules
Spokje	PHPStan has already saved my/our/some behind a few times during the move-core-tests-around in D10 (for example here: #3227033: Remove Quick Edit from core#comment-14389355)

9️⃣ Should the module “removal” issues against 10.x actually be converting them to empty “outdated” stubs like we did in D9?

Gábor Hojtsy (he/him)	Proposed by @xjm
xjm	So, during the D8 to D9 upgrade, we explicitly made simpletest (among other things) an empty stub that uninstalled itself on upgrade, in order to avoid fatal errors when upgrading a site
xjm	I was supposing the process now that we have the new API would be:
catch	For non-dev modules where we're providing a contrib version, I don't think we should. The course of action is to install the contrib module, or uninstall, but if you do neither there's a problem.
xjm	Deprecate in D9Create in contrib and make the stub outdated in D10Remove stub in D11
catch	If it's a dev module, it shouldn't be installed on prod anyway so safe to uninstall in an update. If it's empty and vestigial like entity_reference, also safe to uninstall in an update.
Spokje	@catch commented here about make-nuke-not-stub before: #3227033: Remove Quick Edit from core#comment-14389263INSTA-EDIT: Seems like mentioning [at]catch is much faster than searching through d,o.Didn't realize he was already in this thread. (edited)
xjm	@catch Do you remember why we kept around the empty multilingual and ER stubs then?
xjm	Or simpletest
xjm	Which still exists in 10.0.x HEAD :wink:
catch	@xjm because we can uninstall those without any consequences.
catch	And we have to leave the stubs until after they've definitely been uninstalled.
xjm	Ahh, right, it'd be (potentially massive) data loss for CKE4 to uninstall itself underneath you
xjm	I think we should still have manual testing to ensure site owners understand the errors they do get when they upgrade and the contrib is not there
xjm	We'd only need to do that for the first removal that wasn't an outdated stub
catch	So stubs are only for obsolete (and simple test).
catch	I am 99% sure you get a PHP warning saying $module_name is missing from the filesystem.
xjm	Manual testing and UX review as to whether that's sufficient. :wink: Or maybe just an opinion from @Gábor Hojtsy (he/him) or another product manager after seeing it demoed.
xjm	We probably shouldn't invent some hard-to-maintain thing just to give people a nice message, but I'd like to know whether someone could figure out how to proceed based on what does happen.
Gábor Hojtsy (he/him)	Yeah trying it out would be important >D
xjm	Like reading release notes is unfortunately not a universal. A lightweight mitigation might be special-casing certain module names on extension discovery, or something (not saying we should actually do that; I don't know) (edited)
xjm	To tell people "go install this contrib module" rather than "fatal error rawr" (I don't know if it'd occur to a site owner or not)
larowlan	They can install the contrib module before moving to d10 fwiw
xjm	goes to dig out the Place Block issue
larowlan	i.e. we're recommending they do it in 9.4
larowlan	by way of the lifecycle_link/deprecated status
xjm	@larowlan Yep agreed, but they won't necessarily know to do that from the current messaging
larowlan	the lifecycle link needs to tell them exactly what to do
xjm	Or will ignore their status reports
larowlan	it has composer commands in it
xjm	https://www.drupal.org/files/issues/2021-12-10/2021-12-10_08-37-22.png what Composer commands?
larowlan	clicking more information
larowlan	hmm https://www.drupal.org/about/core/policies/core-change-policies/deprecat... did include composer commands for forum but they've been removed :thinking_face:
xjm	Ah, so each change record would need this info
larowlan	ah we have two lifecycle link pages https://www.drupal.org/docs/core-modules-and-themes/deprecated-modules-a...
larowlan	I'll remove the second one
xjm	"more information" doesn't link that page, though, does it?
andypost	one more question here is should we clean-up "replace" composer section when we remove modules? as I see "drupal/migrate_drupal_multilingual": "self.version", still here
xjm	It links to lifecycle_link which I thought was a CR
xjm	Maybe it's not
larowlan	lifecycle links go to handbook pages, not CRs
larowlan	that was how catch recommended them
xjm	@andypost That's a failure in the removal issue; hotfix or revert would work
Gábor Hojtsy (he/him)	Yeah they are end user facing links shown on the UI
xjm	I just checked and migrate_multilingual did link a CR, not this handbook page
larowlan	ok, tracker + forum + hal are linking to this page
xjm	entity_reference OTOH links https://www.drupal.org/about/core/policies/core-change-policies/deprecat...
larowlan	should probably use an nid instead of path alias, unless d.o has that redirect feature where past aliases keep resolving
larowlan	so I guess we also need an issue to update the migrate multilingual drupal lifecycle link
xjm	Also, should it even be a separate API if we want to standardize on a single, best-practice page? Or is the idea that contrib should be able to link to something else?
larowlan	Yeah contrib can link elsewhere
xjm	We could add to ModuleInstallUninstallTest (I think) verifying that any core modules link the right place
larowlan	Would be simpler to create a separate test, ModuleInstallUninstallTest is a beast and takes 9mins to run on my local
larowlan	could just be a kernel test that used extension discovery to find deprecated modules
larowlan	But +1 for a test
xjm	Ah, yeah, nothing that it needs a full site for, only needs the info files really.
xjm	Anyway, the lifecycle links will only be in 9.4.x for the actual "site is broken without me" core module removals
xjm	Although that brings up the whole point of whether site owners need to upgrade to 9.4.x before 10.0.x which is another whole thing. We're deprecating so much stuff in 9.4.x that being on 9.3.x probably isn't enough, but OTOH folks who do plan to adopt D10 right away might not want to do two upgrades in a row.
catch	It might make sense for obsolete modules to link to a change record, since they're not supposed to show in the user interface anyway, and then modules moved to contrib to link to this page. I didn't have a strong opinion on CR vs. this page, but it seemed like it would help to avoid duplication if we went to the documentation page and we already had a module doing it.But if we allow core to potentially link to two places we can't test that.
larowlan	Especially if there's more than one deprecated, having it in one spot is a bit nicer for site owners