dww: Stubs in D10 to uninstall obsolete versions of things we removed? xjm: Stubs are only for defunct things not moved to contrib dww: @longwave mentions this multiple times: * Remove Color * Remove Quick Edit xjm: Like SimpleTest, Entity Reference, etc. xjm: Hmm, good questions... xjm: summons @catch to thread dww: Yeah, I was concerned about a conflicting DB update in core that then breaks contrib QE in some way. xjm: I think we should maybe make the update instructions say "You MUST uninstall this OR install the contrib version", and possibly not support the "I completely refuse to read instructions" implementation. dww: But right, the more common case will be sites upgraded to D10 w/o installing contrib QE or whatever. ... dww: I can't imagine relying on sites to RTFM is going to work. xjm: Well it's at least a nice clean fatal error xjm: There's no way to upgrade and not notice that you screwed it up xjm: I think for the ones that are already deprecated they are gone-gone in 10.0.x; checking xjm: [ayrton:drupal | Wed 17:26:02] $ ls core/modules/aggregator ls: core/modules/aggregator: No such file or directory xjm: [ayrton:drupal | Wed 17:26:23] $ ls core/modules/hal ls: core/modules/hal: No such file or directory xjm: Yah so to date no stubs dww: I won't do anything with the QE MR until y'all decide for sure if this means we forgot a step for the ones we already did, or if Dave is inventing steps we don't need for the ones still in progress. :sweat_smile: longwave: Simpletest was moved to contrib which is what made me think of doing this (albeit still effectively abandoned) dww: Indeed, 9.5.x core/modules/simpletest is definitely there. dww: (as a counter-example to the 10.0.x hal + aggregator results above) xjm: Yeah we did that because of CIs and because having it at runtime was the edgecase for SimpleTest longwave: For RDF we imagine many users have it because of Standard profile yet don't really know what it does so to me that is a reason to uninstall it for them longwave: QE and Color are much more functional/visible so that might be a different story xjm: My only worry is deleting people's data with the automatic uninstall dww: I forgot, does having something with lifecycle: obsolete mean it's immediately uninstalled? Otherwise, I don't even see how what we've got in core/modules/simpletest forces it to uninstall itself. xjm: We implemented a hook for that xjm: (Before the API key existed) xjm: We would still need to; it doesn't happen automatically AFAIK xjm: (Or if it does, I missed that issue) longwave: system_post_update_uninstall_simpletest() dww: AFAICT, Quick Edit has no user data to lose or delete. xjm: Yah ^^ that xjm: Yeah I'm not too fussed about QE, only modules that have config or content that would go bye-bye dww: Unless you consider the perms data... dww: But a system_post_update_uninstall_quickedit() would be a big problem for sites that did want to transition to contrib, right? xjm: I wonder if we could add conditional logic to the hypothetical post-update that checks for a contrib version before running. Logic for that must already exist somewhere in the module handler. xjm: (Might not have API, but at least the logic pattern) xjm: I think Nat and I did discuss this once before and Nat had suggested no stubs for runtime contrib project replacements, but I don't remember specifics anymore. dww: I guess we should check what currently happens if you've got 9.4.x core QE enabled, don't install contrib, and upgrade to D10. dww: And QE is Simply Gone(tm). dww: Do we in fact fatal error, and does the message make sense to be able to recover from? xjm: There's probably the result of that in the test results from my original "Test nuking QE" MR. One mo... longwave: The QE removal issue fails in the upgrade tests exactly for this reason longwave: IIRC there is an error about quickedit being missing but I'm on phone and about to go to sleep so can't check longwave: Color had the fixtures rerolled so it was no longer installed (and so the tests pass) but I think this is not right longwave: I guess there is the same question about themes longwave: If someone is using Seven do we force them to Claro or fail hard? dww: Right, I must admit the whole idea of re-rolling the "stable" DB fixtures to pretend we had never installed these things isn't the most realistic test case. (edited) dww: Meanwhile, from the manual testing window... % composer require drupal/core-composer-scaffold:10.0.x-dev drupal/core-project-message:10.0.x-dev drupal/core-recommended:10.0.x-dev drush/drush -W ... % rm -rf web/core/modules/quickedit % drush updb [error] (Currently using Missing or invalid module The following module is marked as installed in the core.extension configuration, but it is missing: * quickedit Review the suggestions for resolving this incompatibility [1] to repair your installation, and then re-run update.php. [1] https://www.drupal.org/docs/8/update/troubleshooting-database-updates ) Requirements check reports errors. Do you wish to continue? (yes/no) [yes]: dww: But, heh: [success] No pending updates. dww: (Is that expected? Upgrading from 9.4.5 to 10.0.x-dev there are no updates?) dww: Umami test site page reloaded just fine. dww: QE is now gone from contextual links, but no other sign of trouble xjm: OK that manual testing is super clear xjm: Question is, does the UI have the same messages or is it just a drush feature? dww: /drupal-quickedit/web/en/admin/reports/status now shows: Drupal Version: 10.0.0-dev Errors found Missing or invalid module The following module is marked as installed in the core.extension configuration, but it is missing: quickedit Review the suggestions for resolving this incompatibility to repair your installation, and then re-run update.php. dww: I'll try a core UI upgrade, 1 sec. ... catch: No stubs! (Catches up with rest of thread) catch: You should get PHP warnings all over the place if a module is missing and that that point you can either roll back or install the contrib module after googling. catch: If we stubbed it would hide the problem xjm: There were apparently no warnings with QE, just a status report message (although it's only on special paths). xjm: But a very clear one. ... dww: Ok, so drush updb prints the requirements error but lets you proceed. That's the weirdness. /drupal-quickedit/web/update.php OTOH finds the requirements error and forces you to fix it: Check the messages and try again. Screen Shot 2022-08-17 at 4.10.05 PM.png xjm: That's perfect! xjm: Go core UI xjm: We should file an issue against drush for this maybe catch: There might already be one, this has come up. catch: But also there might not be... xjm: There was a different one for PHP install warnings not being respected by drush (edited) xjm: (Doesn't mean there wasn't also this issue; that's just the one I remember in the past year) ... dww: Ahh, and this sort of what https://www.drupal.org/project/drupal/issues/3294914 is trying to improve. Drupal.org Create dedicated error section for missing removed core modules on update Problem/Motivation In D10.0.0 we will have the situation that people had deprecated in D9.x and now removed in D10.0.0 core modules enabled. Whenever this happens, currently those missing modules are detected in update.php, but these aren't a special case. They are handled in the same way a missing contrib module would and the link given (https://www.drupal.org/docs/8/update/troubleshooting-database-updates) in the error message doesn't mention deleted core modules. Let's make a dedicated error message with some more specific help text/links. dww: (by linking to another page entirely, among other things) xjm: @longwave So I am concluding from the above that we probably don't need stubs, unless some specific problem comes up in manual testing that breaks the above UI and drush warnings. xjm: And then just file a drush issue to stop bypassing core errors