CachedFilesMixin Doesn't Limit Substitutions to Extension Matches

[Matthew Tretter]

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).

[Simon Meers]

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

[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.

[Simon Meers]

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.

[Simon Meers]

WIP patch

[Stavros Korokithakis]

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

[Claude Paroz]

Patch updated in this ​PR. Tests fail on Windows.

[Claude Paroz <claude@…>]

In edcecaf0:

Fixed #19670 -- Applied CachedFilesMixin patterns to specific extensions

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