Opened 4 years ago

Closed 6 months ago

#19670 closed Bug (fixed)

CachedFilesMixin Doesn't Limit Substitutions to Extension Matches

Reported by: Matthew Tretter Owned by: Claude Paroz <claude@…>
Component: contrib.staticfiles Version: 1.4
Severity: Normal Keywords:
Cc: 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 format of the patterns attribute seems to imply that substitutions will only be performed in files with the corresponding extension. However, if a file matches *any* of the filename patterns, then *all* of the listed substitutions will be performed. For example:

    patterns = (
        ("*.css", (
            r"""(url\(['"]{0,1}\s*(.*?)["']{0,1}\))""",
            r"""(@import\s*["']\s*(.*?)["'])""",
        )),
        ("*.js", (
            r"whatever",
        ),
    )

Not only will url_converter() be called for any occurrences of the word "whatever" in JavaScript files, but also for any occurrences of url('something') and @import('something') (in JavaScript files).

Attachments (1)

19670.diff (4.7 KB) - added by Simon Meers 3 years ago.
WIP patch

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by Carl Meyer

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 3 years ago by Simon Meers

Owner: changed from nobody to Simon Meers
Status: newassigned

I have a patch for this, just need to put the tests together.

comment:3 Changed 3 years ago by Stavros Korokithakis

We are affected by this. It seems that changing for patterns in self._patterns.values(): to for patterns in self._patterns.get(extension, []): will do the trick.

comment:4 Changed 3 years ago by Simon Meers

Owner: Simon Meers deleted
Status: assignednew

Just in case I don't get around to finishing this, I'll attach my WIP patch. From memory it was all working fine apart from an isolation issue whereby an unrelated test method would cause failure depending on the order in which the tests were run.

Changed 3 years ago by Simon Meers

Attachment: 19670.diff added

WIP patch

comment:5 Changed 3 years ago by Stavros Korokithakis

Looks correct to me. We'll try this out in our code and report back tomorrow, thanks.

comment:6 Changed 7 months ago by Claude Paroz

Has patch: set
Patch needs improvement: set

Patch updated in this PR. Tests fail on Windows.

comment:7 Changed 6 months ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:8 Changed 6 months ago by Claude Paroz <claude@…>

Owner: set to Claude Paroz <claude@…>
Resolution: fixed
Status: newclosed

In edcecaf0:

Fixed #19670 -- Applied CachedFilesMixin patterns to specific extensions

Thanks Simon Meers for the initial patch, and Tim Graham for the review.

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