Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30263 closed Cleanup/optimization (fixed)

Mention the Media.merge change in the release notes

Reported by: Matthias Kestenholz Owned by: Carlton Gibson
Component: Forms Version: 2.2
Severity: Release blocker Keywords:
Cc: Johannes Maron Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The Media merge change (#30179, #30153) has some potential of breaking libraries which worked before. It would be nice to give a heads-up to developers that they have to check their Media definitions.

The necessary change to django-content-editor was really small: https://github.com/matthiask/django-content-editor/commit/ed662db302fd74033ce62031bb11b6f8beddf249

Here's a draft for the release notes (I hope it helps -- I'm finding it hard to keep it short while still providing an useful example)

--

Merging of media classes

The Media class merging algorithm has been changed to produce sensible results when pairwise merging is insufficient. However, CSS and JavaScript files which do not properly declare their dependencies may now be reordered before their dependencies. This only ever worked by accident.

The recommendation is to check all Media declarations for dependencies and make those explicit. Just as an example, widgets depending on django.jQuery should specify js=["admin/js/jquery.init.js", ...]

Change History (11)

comment:1 by Carlton Gibson, 5 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Hey Joe. Yep, fine. Do you want to make that PR? (I can do it quickly if you say no.)

comment:2 by Matthias Kestenholz, 5 years ago

Hey, that was me :)

I'd appreciate it if you did the PR. I just thought I'd cc Johannes because of his involvement.

comment:3 by Johannes Maron, 5 years ago

Hi @Matthias,

thanks for letting us know about this. I believe it's a good idea to add some documentation here. Since other might rely on implicit dependencies especially with the old merging warning.

Since you encountered the issue, what would you suggest as documentation. What would have helped or pointed you into the right direction?

Best,
Joe

comment:4 by Carlton Gibson, 5 years ago

Sorry. I misread the UI. I'll make a PR this PM. Thanks both.

comment:5 by Matthias Kestenholz, 5 years ago

Thanks, both of you.

I'd add the sentence in the ticket description under the "forms" heading and also mention the change under the "admin" heading, coupled with the recommendation of adding jquery.init.js to the js list.

(It didn't confuse me this time since I authored the last change to Media.merge. A hint with what-to-do / upgrading instructions would be very helpful though. Upgrading instructions for breaking changes is a thing I missed in the past, e.g. when get_query_set was changed to get_queryset etc.)

comment:6 by Carlton Gibson, 5 years ago

Owner: changed from nobody to Carlton Gibson
Status: newassigned

comment:7 by Carlton Gibson, 5 years ago

PR with ≈suggestion from Matthias.

comment:9 by Carlton Gibson, 5 years ago

Has patch: set

comment:10 by Tim Graham, 5 years ago

Triage Stage: AcceptedReady for checkin

comment:11 by GitHub <noreply@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 418263c4:

Fixed #30263 -- Doc'd changes to form Media sorting (refs #30179).

Thanks to Tim Graham for review.

comment:12 by Carlton Gibson <carlton.gibson@…>, 5 years ago

In 19ab6989:

[2.2.x] Fixed #30263 -- Doc'd changes to form Media sorting (refs #30179).

Thanks to Tim Graham for review.
Backport of 418263c457636d3301f2068c47f09a0f42e15c52 from master

Note: See TracTickets for help on using tickets.
Back to Top