Problem/Motivation

In #2880237: [meta] Refactor system/base library, #3432183: Move system/base component CSS to respective libraries where they exist and #2484623: Move all JS in modules to a js/ folder we're moving files between library definitions and/or to different locations in the filesystem.

This is allowable under the bc policy (file locations and data structures aren't an API), but it also definitely will break library-overrides when we do it.

Steps to reproduce

Proposed resolution

Add a 'moved_files' key to library definitions. This is specified in the library that the file as moved to or within, not the file that it has moved from.

The moved_files key has entries for css and js, those entries are the old filename, and the location of the new filename in the same format as they would be specified. Examples in the MR.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3432601

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.

catch’s picture

catch
he/him
Primary language English
commented
Title: Add a bc layer for library-overrides » Add a deprecation support for library-overrides
catch’s picture

catch
he/him
Primary language English
commented
Title: Add a deprecation support for library-overrides » Add deprecation/bc support for library-overrides when files are moved
Status: Active » Needs work

Got something working.

The main drawback is that the deprecation versions apply by library rather than file. However, moving more than one file to the same library in different versions doesn't seem that likely - and the removal version ought to be the same in those cases. Trying to do the deprecation version per-file would make the already complex YAML format even more complex.

catch opened merge request !7132

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

Hoping to get a nice deprecation failure on a test somewhere so we can just add more expectDeprecation() rather than a whole new test case.

Updated the issue summary a bit. Will need docs and change record but would like reviews before that in case the format ends up changing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Deprecation support is in with test coverage now.

The one limitation of the deprecation support is you can only have one version/change record per library that you've moved files from - however moving multiple files to and from the same libraries in different releases seems like an extreme edge case. If you move multiple files from the same library (like system/base) to lots of different libraries in different versions, it will support that fine because it's the libraries you move files to that determine the messages.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work 

moved_to:
  version: VERSION
  css:
    base:
      css/base-remove.css: {}
  js:
    js/foo.js: {}
  moved_files:
    theme_test/moved_from:
      deprecation_version: drupal:X.0.0
      removed_version: drupal:Y.0.0
      deprecation_link: https://example.com
      css:
        base:
          css/foo.css:
            base: 'css/base-remove.css'
      js:
        js/bar.js: 'js/foo.js'

So not sure I fully follow

"moved_to" is the new library
Then are we saying we are moving files from "theme_test/moved_from"

Should the deprecation links be on "theme_test/moved_from" though? The deprecated key makes it sound like we are removing these files from the library.

But moving to NW for the change record as that might help clear it up.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Should the deprecation links be on "theme_test/moved_from" though? The deprecated key makes it sound like we are removing these files from the library.

We only get library info for libraries that are requested during the request, when 'moved_from' is requested (if that ever happens), it doesn't matter that some code used to override a file in it, because the file is no longer there to be overridden.

However when the 'moved_to' library is requested, that includes the file that the theme is trying to override - so we need to warn the theme then.

Filled out the change record a bit.

smustgrave’s picture

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

Thanks, reading the CR and that does help clear things up.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

I left some comments in the MR.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Reworked the deprecation messages, I was trying to keep the maximum amount of information, but having them readable by a human is more important.

smustgrave’s picture

smustgrave commented

Does the CR need to be updated with new message?

catch’s picture

catch
he/him
Primary language English
commented

No the wording isn't in the change record.

smustgrave’s picture

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

Gotcha!

catch’s picture

catch
he/him
Primary language English
commented

Added the moved_files entries on #3432183: Move system/base component CSS to respective libraries where they exist.

quietone’s picture

quietone commented

@catch, thanks for adjusting the deprecation message.

There were conflicts so I did a rebase. There was one conflict in LibraryDiscoveryParser in the use statements. It was a simple fix so I leaving this at RTBC.

  • nod_ committed 5254c779 on 11.x 
    Issue #3432601 by catch, quietone, smustgrave: Add deprecation/bc...

  • nod_ committed d460133a on 10.3.x 
    Issue #3432601 by catch, quietone, smustgrave: Add deprecation/bc...

nod_ closed merge request !7132

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 5254c77 and pushed to 11.x. Thanks!

Backported to 10.3.x

Status: Fixed » Closed (fixed)

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