Problem/Motivation

Color module is going to be deprecated in 9.4 and removed in 10 #3270936: Deprecate Color module

Proposed resolution

Remove Color module from the Standard profile.

Remaining tasks

Patch
Review
Commit

User interface changes

no, for new installations and has no effect on existing ones

API changes

no

Data model changes

no

Release notes snippet

Standard profile will no longer enable the color module when installed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

Issue summary: View changes

Remove release not snippet - it was for RDF.

longwave’s picture

Status: Needs review » Active

No patch to review yet!

quietone’s picture

Status: Active » Needs review
FileSize
5.42 KB

Sorry about that. I was making a couple of issue about the same time.

Let's start.

Status: Needs review » Needs work

The last submitted patch, 4: 3270941-4.patch, failed testing. View results

quietone’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
7.08 KB

Changes for the two failing tests to remove color.

bbrala’s picture

This probably needs a reroll after #3270897: Handle migration tests for removing Color is committed.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar

Picking it now.

yogeshmpawar’s picture

Rerolled the patch against 9.4.x branch & added a reroll diff.

bbrala’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/tests/src/Functional/migrate_drupal_ui/d7/MultilingualReviewPageTest.php
@@ -51,7 +51,6 @@ protected function getAvailablePaths() {
-      'Color',

@@ -134,6 +133,7 @@ protected function getMissingPaths() {
+      'Color',

+++ b/core/modules/aggregator/tests/src/Functional/migrate_drupal_ui/d7/NoMultilingualReviewPageTest.php
@@ -48,7 +48,6 @@ protected function getAvailablePaths() {
-      'Color',

@@ -136,6 +135,7 @@ protected function getMissingPaths() {
+      'Color',

+++ b/core/modules/aggregator/tests/src/Functional/migrate_drupal_ui/d7/UpgradeTest.php
@@ -122,7 +122,6 @@ protected function getAvailablePaths() {
-      'Color',

@@ -187,6 +186,7 @@ protected function getAvailablePaths() {
+      'Color',

+++ b/core/modules/help/tests/src/Functional/HelpTest.php
@@ -101,7 +101,7 @@ public function testHelp() {
-    $list = ['Block', 'Color', 'Custom Block', 'History', 'Text Editor'];
+    $list = ['BigPipe', 'Block', 'Custom Block', 'History', 'Text Editor'];

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MultilingualReviewPageTest.php
@@ -61,8 +61,6 @@ protected function getAvailablePaths() {
-      // @todo Remove Color in https://www.drupal.org/project/drupal/issues/3270899
-      'Color',

@@ -154,6 +152,8 @@ protected function getMissingPaths() {
+      // @todo Remove Color in https://www.drupal.org/project/drupal/issues/3270899
+      'Color',

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/NoMultilingualReviewPageTest.php
@@ -57,8 +57,6 @@ protected function getAvailablePaths() {
-      // @todo Remove Color in https://www.drupal.org/project/drupal/issues/3270899
-      'Color',

@@ -157,6 +155,8 @@ protected function getMissingPaths() {
+      // @todo Remove Color in https://www.drupal.org/project/drupal/issues/3270899
+      'Color',

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/Upgrade7Test.php
@@ -138,8 +138,6 @@ protected function getAvailablePaths() {
-      // @todo Remove Color in https://www.drupal.org/project/drupal/issues/3270899
-      'Color',

@@ -205,6 +203,8 @@ protected function getAvailablePaths() {
+      // @todo Remove Color in https://www.drupal.org/project/drupal/issues/3270899
+      'Color',

--- a/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php
+++ b/core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php

These changes seem out of scope. These look like re-orders.

I think the only thing that needs to change is removing is from the standard.info.yml and PageCacheTagsIntegrationTest.

bbrala’s picture

Issue tags: +Needs change record

This also needs a change record i think. But will ask around to verify.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
2 KB
5.58 KB

Made changes as per comment #10.

bbrala’s picture

bbrala’s picture

Status: Needs review » Needs work
+++ b/core/modules/help/tests/src/Functional/HelpTest.php
@@ -101,7 +101,7 @@ public function testHelp() {
-    $list = ['Block', 'Color', 'Custom Block', 'History', 'Text Editor'];
+    $list = ['BigPipe', 'Block', 'Custom Block', 'History', 'Text Editor'];

Why id BigPipe suddenly appearing here? This feels like out of scope as mentioned in #10.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
715 bytes

Addressed comment #14.

The last submitted patch, 12: 3270941-12.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 15: 3270941-15.patch, failed testing. View results

bbrala’s picture

I think I was very wrong about my comments regarding the unrelated changes. Sorry about that, think my mind was in the mindset of removing this module from core, which is NOT what this issue is about. Sorry about that.

Seems like the patch in #9 is correct. But I don't trust myself anymore today :)

bbrala’s picture

Status: Needs work » Needs review
yogeshmpawar’s picture

Thanks @bbrala. I am removing other patches after #9

xjm’s picture

+++ b/core/modules/help/tests/src/Functional/HelpTest.php
@@ -101,7 +101,7 @@ public function testHelp() {
+    $list = ['BigPipe', 'Block', 'Custom Block', 'History', 'Text Editor'];

Is the addition of BigPipe here intentional? Edit: I see this was asked before, but not actually answered. Can we explain why BigPipe needs to be added?

quietone’s picture

@bbrala, you are correct, they are related changes. I agree that working on the different issues concerning 'remove a module from core' does get confusing.

10. The changes to the migration tests are necessary because these tests install the standard profile which will install Color and effect which the list of modules that are upgraded or not upgraded.

10 and 21. The change to HelpTest was done because it was testing sorting and I wanted to keep two items in the list starting with the same letter. This is no longer needed as a previous patch added 'Breakpoint'. The other issue is #3270940: Move all non migration Color tests to the module in preparation of removal.

I started with the patch in #9, rerolled and removed the change to HelpTest.

Status: Needs review » Needs work

The last submitted patch, 22: 3270941-22.patch, failed testing. View results

quietone’s picture

Missed updating RDF.

andypost’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Updated IS, patch looks good, CR exists

not sure about release notes snippet

murilohp’s picture

Hey @andypost just trying to help you out here, I've created this new patch for D10 only since #24 is not applying.

murilohp’s picture

Status: Reviewed & tested by the community » Needs review

Moving back to NR, to see what you think about the reroll.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, RTBC again (there's aggregator in D10)

  • catch committed bf76bf1 on 10.0.x
    Issue #3270941 by quietone, ravi.shankar, yogeshmpawar, murilohp, bbrala...

  • catch committed 0ad6705 on 9.4.x
    Issue #3270941 by quietone, ravi.shankar, yogeshmpawar, murilohp, bbrala...
catch’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

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

andypost’s picture

Thank you! the last step is RTBC - #3270936: Deprecate Color module

I'm working on moving code

quietone’s picture

Status: Fixed » Needs work

The Issue Summary and comments didn't make it clear that there were two patches here, one for D9.4 and one for D10. As a result the D10 patch was committed to D9.4 and the changes to aggregator were missed.

I reckon that on 9.4.x 0ad6705 can be reverted and then the patch in #24 can be committed.

heddn’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
2.57 KB

Bumping to critical as 9.4 tests are currently failing. And here's a patch with the rollback and application of #24.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

As #34 said 9.4 needs this

yogeshmpawar’s picture

+1 RTBC

  • catch committed 0e319b4 on 9.4.x
    Issue #3270941 by quietone, ravi.shankar, yogeshmpawar, murilohp, heddn...
catch’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Fixed

Committed 0e319b4 and pushed to 9.4.x. Thanks!

Restoring status.

Status: Fixed » Closed (fixed)

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