There is logic in the handling of image assets for the various apps (screenshots, icons) that currently depends on the last-modified header from the webserver that might not be present (http://tools.ietf.org/html/rfc2616) in all cases. There is currently an error thrown #1763394: Notice: Undefined index: last-modified in apps_retrieve_app_image when it isn't supported.

CommentFileSizeAuthor
#4 1790902.4-check-last-modified-existing.patch702 bytesmrfelton
#1 1790902-check-last-modified-existing.patch1 KBpopulist
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

populist’s picture

populist CreditAttribution: populist commented
Status: Active » Needs review
FileSize
1790902-check-last-modified-existing.patch1 KB

This patch will do a simple check to see if the last-modified value exists before doing checking against it. It also changes and documents the conditional logic so the "file" is altered if either information (last-modified or content-length) exists but is different than the original file.

elliotttf’s picture

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

Is there a chance that $remote['last-modified'] could legitimately be 0 (not necessarily intentionally, but perhaps due to server misconfiguration, etc) in which case isset() should be used in favor of empty()?

That edge case aside this change makes sense and looks good to me.

populist’s picture

populist CreditAttribution: populist commented

@elliotttf - It is not possible for last-modified to be legitimately zero since the spec requires an HTTP-date (http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.3.1) which cannot be zero.

mrfelton’s picture

mrfelton CreditAttribution: mrfelton commented
Status: Reviewed & tested by the community » Needs review
FileSize
1790902.4-check-last-modified-existing.patch702 bytes

Same patch, updated to apply against latest codebase

mrfelton’s picture

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

Leaving as RTBC, since this is just a straight reroll.

hefox’s picture

hefox CreditAttribution: hefox commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

That wasn't the straightiest of rerolls, populist have you tested it?

hefox’s picture

hefox CreditAttribution: hefox commented
Status: Needs review » Fixed

Seemed safe enough

  • hefox committed 0fa14de on 7.x-1.x 
    Issue #1790902 by mrfelton, populist: Handling Cases Where Last-Modified...

Status: Fixed » Closed (fixed)

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