Summary

This proposed policy comes from a discussion in #3050374: Create Drupal 9 stable theme

In Drupal 8 the stable theme includes copies of all image assets from modules and core/misc. In the process of creating the Stable 9 theme, @lauriii and @bnjmnm wondered if this copying of images is necessary or helpful as multiple copies of the same image is confusing for developers and adds to Drupal's download size. Plus, in the case of Drupal 9, this would result in three copies of the same image assets, the originals, copies for the deprecated Stable (8), and copies for Stable 9. Sincere attempts were made to track down the rationale for copying all images, but nothing more detailed than #2575737: Copy templates, CSS, and related assets to Stable and override core libraries' CSS could be found, which approached the image copying as a given.

The proposed solution is:

  • Do not add copies of image assets to Stable 9 or future versions of stable
  • When a Stable theme is moved to contrib, the images should be copied to the theme
  • In the event that an image in misc or modules is changed, the original version of that image should be added to Stable to ensure backwards compatibility. Based on a git history review this occurs infrequently at most, but needs to remain an option.

With this solution we hope to reduce the overhead of multiple copies of identical files, while still preserving backwards compatibility. This will inform how the Stable 9 theme is created.

As a followup, implement #3107186: Add a test to ensure that image assets are copied to Stable when a module or misc asset is updated but not as a requirement to this decision.

Release notes snippet

The Stable theme no longer includes copies of all images in /misc and /modules. To maintain backwards compatibility, when one of these images is changed, its prior version will be copied to Stable.

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Related issues: +#3050374: Create Drupal 9 stable theme
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue summary: View changes
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

Discussed this with @xjm and @catch on last weeks committer meeting. We didn't come to a definitive conclusion on whether we should include image assets in the Stable theme or not. There was some concerns on whether we would actually remember to copy assets to stable when we make changes to them. On the other hand, we didn't make any changes to image assets during the Drupal 8 life cycle so it seems redundant to keep copies of the exact same files in multiple places.

Maybe we should move forward not copying the assets. I think we could open an issue against the committer hooks to see if we could come up with a way to check this in the hooks. However, I personally wouldn't make that a requirement for #3050374: Create Drupal 9 stable theme.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

There was some concerns on whether we would actually remember to copy assets to stable when we make changes to them.

This could be enforced via a unit test that compares md5 hashes of the current images against ones hard-coded in the test. The biggest inconvenience would be that the test would need to be updated anytime a new image is added to a core module or misc/icons, but the fail message could make it easy to know what needs to be updated in the test. Something in the hooks would probably be more efficient, but the unit test approach is an option.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

I like the idea of making it a test because that makes the test available for people not running the hooks as well. If we go with that approach, I think it's important that we also have a test that ensures that all images are listed in the test. However, I'm still fine with this being a follow-up rather than a blocker for the main issue given that this is a pre-existing issue.

xjm’s picture

xjm
she/her
Primary language English
commented

Regarding #4 and #6, I'm also fine with this being a followup, scope something like "Ensure that image assets are copied to Classy when a module asset is updated" or such?

I'm also 👍for going ahead with this, if we add a CR and perhaps a release note.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

@xjm Any thoughts on including this information in a CR where we introduce Stable 9 since this change is only affecting Stable 9?

xjm’s picture

xjm
she/her
Primary language English
commented

@lauriii Yep that makes total sense; let's attach it to this issue.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Active » Reviewed & tested by the community

Awesome! I think in that case we're done!

I added a step to the issue summary in #3050374: Create Drupal 9 stable theme to make sure this gets done there.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs change record, +Needs release note

We still need the CR and release notes here.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue summary: View changes
Related issues: +#3107186: Add a test to ensure that image assets are copied to Stable when a module or misc asset is updated

Created #3107186: Add a test to ensure that image assets are copied to Stable when a module or misc asset is updated
Created CR
Added release notes snippet.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Thanks!

One more thing I think we need to do is to add this information to: https://www.drupal.org/core/d8-frontend-bc-policy

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs change record

How about we add following text to the policy:

Image assets
Image assets in core /misc folder and modules are considered internal and may change. We will provide backwards compatibility for icons that are used by our stable base themes (Classy and Stable). Themes requiring backwards compatibility, should extend one of those themes.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

The text in #14 looks like it says what is necessary, my only suggestion is a slight rephrasing:
Image assets
Image assets in core's modules and /misc folder are considered internal and may change. We will provide backwards compatibility for images used by our stable base themes (Classy and Stable). Themes requiring backwards compatibility should extend one of those themes.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

+1 to #15. I'm wondering if we should add that to https://www.drupal.org/core/d8-frontend-bc-policy given that it's specifically about Drupal 8? Should we create a new page for Drupal 9?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Re: #16
One of the considerations I had when reviewing #14 is if it accurately described the BC policy for both Drupal 8 and 9, and I believe it does. In both versions, the stable base themes fence in image assets for backwards compatibility purposes. How this is achieved (very pre-emptively in D8 vs. as needed in D9) isn't really necessary for that page since it's describing the assurances provided.

If there is a need to distinguish the D8/D9 approaches, I think it may best be done within the same page as the pages would be largely identical and this would keep things navigable and search-engine friendly. The need for D9 specific pages in this section may be necessary, but an audit of the overall section would inform how to best structure this.

catch’s picture

catch
he/him
Primary language English
commented

+1 to copying assets only if we have to, and the suggested wording for the policy looks good.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes

I was missing a critical part of the proposed resolution, which formed in the comments, added this to the issue summary. With that I think its fine. Later on this test (if/once written) will uncover problems if there are any:

As a followup, implement #3107186: Add a test to ensure that image assets are copied to Stable when a module or misc asset is updated but not as a requirement to this decision.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
commented

I think all that remains is to update the docs and then mark this fixed.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Added text from #15 to https://www.drupal.org/core/d8-frontend-bc-policy. Thank you everyone for the input!

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: -Needs release note