Code

Opened 22 months ago

Closed 22 months ago

Last modified 4 months ago

#18958 closed New feature (wontfix)

CachedStaticFilesStorage should handle errors with broken CSS

Reported by: yxven Owned by: nobody
Component: contrib.staticfiles Version: 1.4
Severity: Normal Keywords: CachedStaticFileStorage
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I am trying to deploy my website. My website makes heavy use of the Dojo Javascript Toolkit. If you're not familiar with it, dojo is a massive javascript library with thousands of files. Every file is not production ready. There are experimental sub-libraries.

When I try to collectstatic this library, it constantly crashes at broken css links to images that don't exist. These are not broken links I want to fix. These are links that are only required by broken experimental plugins to experimental classes that I am not using. This adds to my workload because I now have to go through the library deleting anything I don't need.

What happens in the future when I decide to use one of these classes? If I'm lucky I can figure out exactly what I need and replace it, but in reality, I'm going to have to recopy the library and hack out everything I don't need. Alternatively, I can use code repository tricks to try to replace only the code I needed.

What if I can't just delete the css file? What if I'm using parts of it elsewhere? I'd probably manually edit out the offending line. What if I need the widget that line referred to in the future? It's now going to fail silently. Unless it's completely obvious, I'm not going to notice because I've never used that widget before.

What CachedStaticFilesStorage should do is blindly copy the files while mangling the names with the md5 hash. If there's a broken css link, it's not CachedStaticFileStorage's problem. Alternately, it should warn the user with a dialogue to proceed anyway instead of crashing.

Attachments (0)

Change History (13)

comment:1 Changed 22 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

blindly copy the files while mangling the names with the md5 hash

It is rather difficult to compute the md5 hash of a file when the file doesn't exist. By design CachedStaticFilesStorage isn't usable with CSS that's broken in this way.

However, a better error message pointing to the offending source file and missing target file would help.

comment:2 Changed 22 months ago by aaugustin

  • Summary changed from CachedStaticFilesStorage should not parse CSS to CachedStaticFilesStorage should handle errors with broken CSS

comment:3 Changed 22 months ago by yxven

I didn't realize CachedFileStorage was modifying the css to fix the links. I understand that must be necessary for CachedFileStorage to function.

That said, I was unable to triage my dojo installation by deleting css files. There were too many dependencies.

It should just warn you that the link will be broken. I should be able to tell it to ignore the broken link and keep collecting.

comment:4 Changed 22 months ago by jezdez

  • Resolution set to wontfix
  • Status changed from new to closed

No, it shouldn't just warn that the links is broken.

It was a distinct design decision to only cache files that refer to other files correctly. If the sub-libraries are so experimental that images are missing I would call them broken and not supposed to be deployed by the cached storage backend in the first place.

comment:5 Changed 20 months ago by matthewwithanm

IMO this design decision is misguided. Yes, the CSS files are broken in the sense that they contain broken links, however broken links don't make for invalid CSS. It doesn't seem right to have what's essentially an optimization step place stricter requirements on the source than the language itself.

Practically speaking, given how little browsers complain, this kind of error is very easy to miss—and therefore very likely to make it into (even popular) third-party libraries. (I ran into this issue with django-cms.) Yes, this is an error in the CSS file, but the current behavior of CachedStaticFilesStorage changes the nature and severity of that error.

comment:6 Changed 20 months ago by aaugustin

Yes, I've also encountered the problem with django-cms before. I just submitted a pull request: https://github.com/divio/django-cms/pull/1508

comment:7 Changed 15 months ago by anonymous

I agree it's misguided not to "fix" CachedStaticFilesStorage in this case.

The decision to be "pure" and require totally clean CSS renders CachedStaticFilesStorage useless. I don't have time to sift through issues in 3rd party libraries right now. So I'm disabling CachedStaticFilesStorage. Which is detrimental to my site as a whole.

Wouldn't the greater good be accomplished by making CachedStaticFilesStorage more permissive?

It's not ideal, but the zen of Pythons says "practicality beats purity". I think this is a case where that principle applies.

comment:8 Changed 15 months ago by aaugustin

"Greater good" sounds cool, but that doesn't make it possible to compute the MD5 hash of a file that doesn't exist!

comment:9 Changed 15 months ago by aaugustin

See #18986 for a follow-up on this issue.

comment:10 Changed 8 months ago by andrewsg

aaugustin, do you think a patch that allowed the user to tell collectstatic (for instance via a command-line --argument) to turn these errors into warnings and just move on, leaving the bad relative URL in place as-is, would be acceptable?

comment:11 Changed 8 months ago by aaugustin

Maybe :)

You could test the waters on the django-developers mailing list.

Last edited 8 months ago by aaugustin (previous) (diff)

comment:12 Changed 4 months ago by anonymous

This comes up all the time in massive libraries like boostrap, jquery-ui, etc... I see no reason or harm done in producing a warning that the file is missing and move on the the next file. The argument, "but that doesn't make it possible to compute the MD5 hash of a file that doesn't exist!" is absurd!!! CLEARLY IF THE FILE DOES NOT EXIST A WARING THAT THE FILE IS MISSING IS PRODUCED INSTEAD OF COMPUTING THE MD5! You know, something like try: apply an MD5 hash, except: print a warning that the file is missing. Further more, I think most people have taken the approach 'So I'm disabling CachedStaticFilesStorage' and find other solutions like django-compressor. The fact is, not enough people are using CachedStaticFilesStorage to care or complain about it, because of this issue. The way it is, django should just depreciate and remove this utterly useless feature or enter a little try/excep and we have a useful built in solution.

comment:13 Changed 4 months ago by aaugustin

You seemed to have missed comment 10 that makes exactly the same point, but politely.

If you feel so strongly about this, write a patch. I don't care much, because my CSS is generally correct.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.