From #1226582: Anonymous users not able to add items to cart via AJAX (several problems combined in one issue)

* Added extra header in _zipcart_phpzip() for the right content type "application/zip" so the browser can open it in the right application.

CommentFileSizeAuthor
#4 zipcart-1228402-add-content_type_header-4.patch1.06 KBxurizaemon
#1 zipcart-1228402-add_content_type_header.patch1.21 KBxurizaemon

Comments

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Status: Needs work » Needs review
StatusFileSize
new zipcart-1228402-add_content_type_header.patch1.21 KB

This patch extracted from the one submitted by Matthias on #1226582: Anonymous users not able to add items to cart via AJAX.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

@Matthias, can you tell why you removed the by reference ampersand from zipcart_filterzip()?

(function zipcart_filterzip(&$files) => function zipcart_filterzip($files)).

I recall that originally this was intended to allow custom site code to require the addition or removal of certain files on add to cart. However, it seems like the reference isn't required with the module as it is. Would be good to know if that was changing or breaking how things worked on your site, or if you just removed it as cleanup because it wasn't necessary?

Matthias Lersch’s picture

Matthias Lersch commented

It was the basic problem but more related to #1144436: Block's download count doesn't remain on page refresh.

There is a PHP-warnings if you add somthing to the cart:
Warning: Parameter 1 to zipcart_filterzip() expected to be a reference, value given in module_invoke_all()
as a result the return value of $files = module_invoke_all('filterzip', $files) in zipcart_add_file_to_cart() is empty.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
StatusFileSize
new zipcart-1228402-add-content_type_header-4.patch1.06 KB

Thanks for that, I committed the related change in #1144436.

For portability I suggest we use file_get_mimetype() instead of 'application/zip' as per this new patch - that way we get the correct mime type for the generated filename, and no changes are required if a new archive method is implemented (eg for tgz).

Matthias Lersch’s picture

Matthias Lersch commented
Status: Needs review » Reviewed & tested by the community

file_get_mimetype() is a good Idea.
for me the patch works fine now

ckng’s picture

ckng
Primary language English
commented

Since this is somewhat related, my suggestion to further improvement is to refactor the code to have
- a #1233304: Reusable file transfer function
- #1233302: User definable download filename
- register_shutdown_function to call a hook to indicate download completion (download counter) and clean up the temp file, #1233306: Hook for download completion

EDIT: filed separate issues

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented
Status: Reviewed & tested by the community » Fixed

Committed: 96627c15 / 3ae117c.

Status: Fixed » Closed (fixed)

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