Problem/Motivation

Per #3270899: Remove Color module from core CssAssetOptimizer::loadFile() is declared as public but is not on the interface, it was declared as public so that color module could call it, but color module is being removed from core.

Steps to reproduce

Proposed resolution

  1. Check for contrib module usages see comment #19
  2. Open an issue against color module to re-implement the functionality it needs see #17
  3. ✅ Mark the method protected. See MR 2662.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3302988

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Novice

rpayanm made their first commit to this issue’s fork.

rpayanm opened merge request !2661

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Active » Needs review

Please review.

rpayanm closed merge request !2661

rpayanm opened merge request !2662

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented

Sorry for the noise, this is the good one.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Status: Needs review » Needs work
Related issues: +#3270899: Remove Color module from core

No problem — the MR looks good! Thanks!

There are a bunch of steps still remaining though — see the issue summary. Do you think you could tackle those too? 🤞

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

mdranove made their first commit to this issue’s fork.

mdranove changed the visibility of the branch 3302988-make-cssoptimizer-loadfile-protected-11.x to hidden.

mdranove opened merge request !15785

mdranove’s picture

mdranove commented
Status: Needs work » Needs review

I took a look at the remaining steps here. For step 1, Check for contrib module usages, I don't think there's a way to 100% do this, but what I did do was grep the contrib directory of a very large local codebase with a lot of contrib modules installed for CssAssetOptimizer, there were no results.

Created 3590780

Went ahead and created a new MR against main.

mdranove’s picture

mdranove commented
Status: Needs review » Needs work
mdranove’s picture

mdranove commented
Status: Needs work » Needs review
Related issues: +#3590780: Don't call CssOptimizer for loadFile

smustgrave changed the visibility of the branch 3302988-make-cssoptimizer-loadfile-protected to hidden.

smustgrave’s picture

smustgrave commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Did a search for CssOptimizer::loadFile( https://git.drupalcode.org/search?group_id=2&page=3&scope=blobs&search=C... and just seems to primarily be forks of the color module and comments.

The one I was a little concern about was advagg but seems to only be a comment.

LGTM

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Let's have a CR here just in case someone else is using it that search didn't find.

I think this is fine to do in 12.x-only. We already have the note on the method that it should be marked protected later.

mdranove’s picture

mdranove commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Added CR

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Restoring status after CR

  • catch committed a486ad91 on main 
    task: #3302988 Make CssOptimizer::loadFile() protected

By: catch
By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !15785

Status: Fixed » Closed (fixed)

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