Remove Color module from core

Unpostponing since #3270941: Remove Color module from the Standard profile is committed

Color module needs to be uninstalled in core/modules/system/tests/fixtures/update/drupal-9.3.0.filled.standard.php.gz, with keeping #3264120-42: Remove aggregator module and our dependency on Laminas Feed in mind.

Attempting that now.

I think this is ready for a review now.

Note: In the original policy issue #2808151: [policy] Move the Color module to a contributed project when Bartik is deprecated it states to (re)move the Color module when the Bartik theme is deprecated. Should we wait until that has happened?

@q0rban: Any particular reason you opened a new MR with exactly the same changes as the original?

Apologies, please ignore. I am trying to figure out why the Tugboat builds are failing with composer issues, and accidentally deleted the Tugboat Preview associated with the other MR. I tried to delete my branch, but that was failing too.

That merge request looks … very complete! 😄

I think you missed one important thing though: core/themes/bartik/color/color.inc can be deleted too 🤓 Marking Needs work for that.

The failures in the PHP 8.1 & MySQL 5.7 updated deps and PHP 8.1 & sqlite-3.27 are suspiciously consistent, suggestive of an actual bug. They both fail with:

Testing Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest
.F.                                                                 3 / 3 (100%)

Time: 03:18.297, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testEditModeEnableDisable
Failed asserting that false is true.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayTestBase.php:122
/var/www/html/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php:244
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

But neither that test class nor any of its parents or traits install the Color module. Therefore these must be unrelated random failures nonetheless 👍

I've removed core/themes/bartik/color/color.inc as per comment #26, please review.

Personally I'm unsure about the removal of core/themes/bartik/color/color.inc, in my tiny mind this would mean Bartik wouldn't work (well) with the Contrib incarnation of the Color Module.

But I'm certainly no FrontEndGuru, so let's not take my take for granted.

#29 Interesting take! But … core does not implement contrib APIs. So the Color module in contrib would have to take care of handling the Bartik integration, based on my understanding.

That would normally be true, but we're on our way towards deprecating Bartik and removing it to contrib too, #3278213: [Meta] Tasks to deprecate Bartik. So it makes sense for color support to stay in the contrib Bartik theme, which will need to be moved out of core. If we remove it now, it'd be a case of adding it back in contrib. For me personally given all that, I'd live it in Bartik.

So now we need a ruling, me thinks: "Should it stay or should it go (now)".

If there was an obvious way for color to provide integration for bartik (like #3265140: Move QuickEditImageController from image to quickedit) then we might be able to move it, but I don't think that's even possible? For me I'd leave it in, and let it be removed when Bartik is.

We should not remove color integration in bartik because this way BC for site-builers will be broken otherwise

Please recall how many sites using bartik+color!

then we might be able to move it, but I don't think that's even possible

It could easily be possible if https://www.drupal.org/project/color implemented it I think. But … that's definitely extra work.

Sorry for the distraction! 🙈

e0c0fd1 already got us back to where we need to be, so back to Needs review 👍

@Wim the only question is how other contrib/custom themes will keep color integration?

Removal of db dump also could be shared with other module removals

The MR needs a rebase.

Grmbl, just my luck, doing a rebase after 13 days, which then during the TestBot run already is outdated.
Let's try that again.

Hmmm, GitLab GUI was adamant that a local rebase was needed.
Looks to me like it wasn't.

Postponed on #3291601: Ensure that color does not get special core treatment per Slack discussion here: https://drupal.slack.com/archives/C014CT1CN1M/p1655750184381089
Also needs manual testing when that blocker lands (see also Slack discussion)

The main thing we need to do before removing them from 10.0.x is ensure that the composer façade naming for them is fixed, and have someone manually test EACH to confirm downloading the contrib module in D9 and installing it works SEAMLESSLY, and that it then does again if the rm -rf core/modules/foo

I see that this issue is postponed on #3291601: Ensure that color does not get special core treatment, but that issue is postponed on the removal (this issue). Does that mean we're actually waiting on the manual test described in #42?

I see no reason to postpone this issue, as it was discussed in slack (3 times at least) we should make this in and then unpostpospone #3291601: Ensure that color does not get special core treatment

In that case, setting it back to needs work, because I think the MR is not mergeable now?

MR!2306 is mergeable, GitLab is a bit easy on declaring it needing a rebase when file-deletions are involved IMHO.

This issue is (at least) postponed on manual testing as stated in #42.

At the moment I'm unsure if Color needs extra attention on the facade-front, but postponed is, IMHO, the correct status, because of the sentence above.

I'll give the whole deprecation/removal thing one more push on the D10-readiness meeting Monday 25th to see what manual testing is needed and try to get some final answers/manpower going.

Until then, it's highly unlikely anything will be committed here, so let's leave the status as-is until Tuesday July 26.
By then I'm planning to have all deprecation/removal issues updated with the latest info gathered in the above mentioned meeting.

#3291601: Ensure that color does not get special core treatment is done, this issue is now postponed on manual testing (See https://drupal.slack.com/archives/C014CT1CN1M/p1658772338310209)

Right...

This needs a reroll on the fixture after #3281434: Update System module tests to not use Bartik and Seven landed on 10.0.x. Since there are a _lot_ of fixture changes pending in different issues and updating a fixture is rather tedious work, I'm going to leave this as is until #3302799: Manually test color module removal has been successfully completed.

AFAICT manual testing should be OK, since that has nothing to do with fixtures and automated tests.

If somebody wants to update the 9.3.0-filled fixture in the mean time: Please be my guest

Fixtures updated since #3302799: Manually test color module removal is going places.

Manual testing was completed in #3302799: Manually test color module removal by @bbrala and myself, so un-postponing this issue and will review shortly.

One question about assets in the stable9 theme on the MR, otherwise no further comments on the code changes.

I also grepped through the codebase for "color" and while there are obviously many false positives relating to this removal, one thing stood out in \Drupal\Core\Asset\CssOptimizer:

   * The returned contents are compressed removing white space and comments only
   * when CSS aggregation is enabled. This optimization will not apply for
   * color.module enabled themes with CSS aggregation turned off.
   *
   * Note: the only reason this method is public is so color.module can call it;
   * it is not on the AssetOptimizerInterface, so future refactorings can make
   * it protected.

I guess we could have a followup to deprecate this and make it color.module's responsibility to handle this itself.

We are also in a slightly weird situation if we complete this but don't finish removing Bartik from core before Drupal 10: since #3281430: Update non-migration Color tests to not use Bartik we have code that supports color.module in Bartik but nothing that tests it, so we won't know if it gets broken during the D10 lifecycle. It's likely not that important but not sure what we can do here if this happens.

We are also in a slightly weird situation if we complete this but don't finish removing Bartik from core before Drupal 10

This would be incredibly tricky if we were actively working on removing color module but not Bartik, but since Bartik deprecation and removal is nearly unblocked too (two issues left), I think it's fine - we should be able to complete that in time for beta.

We know the code currently works in core, and then the same code will work in contrib once everything is there. If we moved things around to avoid temporarily weirdness, we'd have to move it all back again.

Opened #3302988: Make CssOptimizer::loadFile() protected.

Thanks @longwave for the review (and @catch for the words of wisdom)

Postponing this again (since no decent removal issue should go through its lifecycle without a couple of those) on #3302976: Port remaining changes from core.
Any Git Gurus help in there would be appreciated.

Note This MR (against 10.0.x) needs a separate patch against 10.1.x.

Since the MR changes include fixture changes and there are a lot of those floating around in several other issues that are close to being committed, I (=Spokje) have given up on creating the 10.1.x-counterparts of the MR.

By the time this issue is actually not being postponed any more it should be quick and easy enough (#FamousLastWords) to create said 10.1.x-patch by me or basically anybody.

MR has been made for the remaining changes of color. Should be merged pretty soon ;)

Updated fixture after #3290810: Remove updates added prior to 9.4.0 (9.4.4 for ckeditor) and add 9.4.0 database dumps landed.

I would say pp-1 is over eager, the missing commits are test only in contrib and a mr is present. I'd vote just keep moving here.

Feel free to update the status whenever you see fit, but if changed this issue should be NR.

Not so much for the changes in the fixtures, but for the thread in the MR that @longwave brought up in #54:

One question about assets in the stable9 theme on the MR, otherwise no further comments on the code changes.

I addressed that here here.

All other questions he had are (IMHO) answered by @catch in #56.

Agree that this doesn't have to wait for the contrib module to catch up, it's only test changes and they can be applied after removal if needed.

I re-reviewed the whole MR and checked again for stragglers after applying, can't find anything that hasn't been addressed, therefore this is RTBC.

Thank you @Spokje for working on this, especially the persistent rerolling of the fixtures, which is both time consuming and error prone.

Thanks both.

Added 10.1.x patch, which is equal to the MR, besides a change in line-numbers in core/phpstan-baseline.neon

Random fail in a quickedit test; merged 10.0.x for a retest.

Thinking about this again, I'm not sure we've done the right thing with the fixture updates. What about the case where someone upgrades to Drupal 10 but does nothing about Color module? Don't we go down the route of e.g. Simpletest where we leave a stub color.install that cleanly uninstalls Color module as part of the upgrade path, and then we remove that stub in Drupal 11? This will then be overridden if the user installs the contrib module beforehand.

Had a very long and fruitful Slack thread w/ @xjm, @catch, @longwave about this. TL;DR:

catch: No stubs!

Attaching the edited transcript (we went off topic a few times) for posterity.

I haven't actually reviewed or tested this 😅 but back to RTBC if this was the only concern. 😉

Heh, not so fast. 😉 I reviewed the MR and had a question about some test changes that I don't see discussed here. See the unresolved MR thread.

Explanation makes sense, real 7.x sites may or may not have color installed so the feature continuing to cover that case works.

Agreed, I didn't read enough context when I commented. RTBC++!

Thanks / sorry,
-Derek

All the changes here look good.

This is directly blocking #3304256: Remove Bartik from Drupal core however we also wanted to make sure that the Bartik patch landed not too long after this one so we don't have the discrepancy of color module support in core when the module isn't. The MR over there looks good to me though except for the dependency here, so time to get this one in.

Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!