CUNY Academic Commons

It would take some non-trivial work to make this happen. See #2682.

Looks like this would be pretty easy - on the post_updated action, the plugin calls a "generate_pdf_file" function, which includes a file called tcpdf.php, which in turn includes a file called tcpdf_config.php, which defines a constant called PDF_HEADER_LOGO that right now is set in staticly, but I think we could define it based on a plugin option. Right now it's set to the static location of the logo image file.

I think we would just have to go into the plugin's options page and change what's in there now to be integrated with WP's media uploader and save the uploaded image's location in a plugin option which we could then reference in the config file. Seems like that integration w/the media uploader is the hardest part, but there are a bunch of tutorials on it, or maybe someone on the team already has experience with that.

Let me know if that makes sense, and what you think should happen next.

Thanks for looking at this, Dan. The technical details here sound fine. I'm more concerned about what the actual workflow will look like, which I suppose will depend on the details of how you integrate the uploader. I have done this a bit before (as has Ray, who I've added as a watcher) and have found it to be moderately difficult. You might spend a bit of time trying to make it work for your purpose, but if you start hitting walls, it probably makes sense to consider a simple form with an input type="file", and the necessary form-handling logic on the back end.

So I think I have this working now - I added a file upload to the plugin options page, and if someone uploads an image, it'll replace the existing file with the new one, even if the filetype uploaded doesn't match .png, which is what it ends up saved as. The user first has to click "Save Changes," and then "Save and Reset PDF Cache" in order for it to work though, which I think is the case for any formatting changes.

There's all kinds of validation stuff to do still, which I don't have a lot of experience with. It'd be good to limit it to certain file types and a certain maximum file size, and to produce errors for the user based on those tests. I'd be happy teach myself, but it might be faster for someone else to take it on. Let me know!

I think I have this working now using wp_handle_upload (thanks pointing me in the right direction, Boone!). I'm only allowing jpeg, png, and gif files but could change that. One issue that I wasn't able to resolve is that the way the plugin was written, the file it uses for the header logo is hard to update (as in change its location), so instead I just overwrite that same file with the new upload. This means some forced file-type conversion since the original is a png. I tested with uploading other file types though and it seems to work fine.

Also - the error strings that get returned aren't ideal (e.g., "The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form." for when it's too large), but I think they work okay. Let me know if I should work on overriding them or anything else.

Here's the changeset: https://github.com/cuny-academic-commons/cac/commit/6ceb88045c006bc81d31314d018e38b1d41b9782

Thanks, Dan! The upload code itself looks good.

The file location seems a bit funky, though. `WP_CONTENT_DIR` . '/uploads/wp-post-to-pdf-logo.png'` will be shared between all sites on the network. This means that if I upload a custom banner on my site, it'll override yours on a different site, which seems pretty messy. Can we abstract out the upload path generation so that it's multisite-aware? Use `wp_upload_dir()` to get a base upload directory that is specific to the current blog. You'll also need to change the `generate_pdf()` function so that it looks in the right place.

Good point Boone - sorry I didn't catch that. I made some changes that I think should fix the issue - saved the path and the url for the uploaded file in the plugin settings, and changed the generate_pdf_file() function to look for those options instead of the defined HEADER_LOGO constant. Had to add in some testing in case there wasn't a file uploaded yet, too. Let me know if you think this works: https://github.com/cuny-academic-commons/cac/commit/d7dcaa5499961c7d20bb198cabbc4ad61699f499

And thanks again!

Hey Dan - This is looking much better, and I think is probably fine as-is. Here are a few kinda pedantic points to consider - if you think they're good ideas, feel free to explore them:

- Storing the full header file path and URL is redundant in a way that can be harmful in certain circumstances. For example, if someone wants to use our domain mapping feature, it will mean that the URL of their blog changes, which would make the URL stored in the options table incorrect (it would result in a 301 redirect). Storing the full file path can cause problems in a distribution plugin because it's not friendly to site migration: if someone dumps the database tables and imports them to a new server, the paths will probably be wrong. A more flexible option is to store only the filename, and concatenate the path at runtime.
- In fact, though, it may not be necessary to store anything in an option. The original implementation of the plugin used a static filename for the header image. You're storing the filename of the uploaded file. If you adopted the original strategy - rename the file to some standard name when uploading - you wouldn't need to store anything.
- Dynamic path generation would solve another problem: getting rid of old header files. Now, if I uploaded a header logo, but then replace it a number of times with different files, they'll remain in the upload directory. Harmless, but a bit sloppy, at least if you're not going to offer any way to access those old header logos. If you use the same filename, you can overwrite the old file - a self-cleaning feature :)
- There appears to be no fallback image support. That is, if there is no user-provided image, there'll be no image at all. This is OK for the Commons (probably ideal, actually), but may not be great for the distrubition plugin.

Again, none of these are critical, certainly not for use on the Commons. But if we want to prepare a patch to send to the plugin author - which would be very nice to do, I think - implementing the suggestions above would comply better with best practices. What do you think?

Thanks for this feedback, Boone. I think it makes a lot of sense. I made changes so that the plugin doesn't use options to store the header logo, and dynamically generates the path & url for it where it's needed. I just use one file name to store the images, so that they overwrite each other. One question I have - doing that meant renaming everything to wpptopdf-header.png, no matter if the original extension was .gif, .jpg, etc. Is that considered an okay thing to do? Seems to work fine. Here's the changeset: https://github.com/cuny-academic-commons/cac/commit/dd3b1fb5efe730e7b24cb35cd979f993c987ddc8

I had a couple of thoughts related to the fallback image:
1. It might actually be nice to include an option to remove the header image without replacing it, in case a user decides they'd like to have no image after having uploaded one. I'm thinking the best way to do this would be a link-looking line of text that says "Remove header image" that would be hooked up to some javascript that would first remove the image from being displayed on the options page and would also set a hidden checkbox that could parsed by PHP and used to delete the file stored as the header image.
2. I think it could be nice to provide a fallback, but not sure what it would be - there isn't a standardized way of storing site header images as far as I can tell, and I don't think it'd be great to have a "PDF produced by WP Post to PDF" banner, either. Do you have any thoughts?

Thanks for encouraging me to poke around with this.

Awesome - this is looking great.

Regarding the file extension: In most cases, the browser will display the image properly even with the wrong extension, but it's not particularly good practice. I'd suggest either (a) writing logic that enforces that either a png or jpg can be exist at the same time with the file name wptopdf-header, and then detecting which of those is present before outputting the HTML, or (b) enforcing that only a png (or whatever) can be uploaded.

I like your "remove header image" idea a lot. Instead of messing with JS, use a URL query arg `http://foo.commons.gc.cuny.edu/wp-admin/.../?remove_header_image=1` and have a listener hooked to 'admin_init' that will detect this query arg and remove the image as appropriate. Make sure you use WP nonces if you do this - look up usage of `wp_nonce_url()` and `check_admin_referer()`.

I think you're right about default header images. No one will want them. Let's leave it out.

Been working on the remove image link and came up against an issue where the query arg in the URL doesn't go away when you resubmit the form, so if you remove the header image, then upload another one (not sure why someone would do that but I'm sure someone would) and re-submit the form, it just deletes the image again. You'd have to leave the options page and come back in order to be able to upload a new image. Is that okay? Can't figure out an easy way around it.

After handling the form submission, `wp_redirect()` to the same URL, minus the query var.

Oh great that's a huge help! Thanks.

I got the remove header image link working with wp_redirect(), and decided to make it so that users can only upload JPEGs (either jpg or jpeg), which I force to be .jpeg. The message the user sees is a little confusing if they try upload a png, bmp, etc. (says it's for "Security reasons," but I didn't see a clean way of customizing the message for these particular cases. Is it okay to just convert .jpg to .jpeg? Or is that still not recommended. If it's an issue I can work on another solution. Here's the changeset: https://github.com/cuny-academic-commons/cac/commit/b15e0d6c56cb63c75aed6acb95e45534660c588e

Let me know if you think this works!

Oh yes sorry I forgot to add this with my last commit! Here's the changeset, let me know if I implemented this right: https://github.com/cuny-academic-commons/cac/commit/96e570eb0c3b3c016f0c16ed3f84d5d514f8fa7b

Check looks good, but make sure that you use && on line 69 rather than the bitwise operator & - we'll get false positives otherwise :)

Well that's embarrassing. Thanks for catching that, Boone! Fix it and one other similar typo. Should be good now.

https://github.com/cuny-academic-commons/cac/commit/d33ff621621b71b5626eb0a9b81bdacafcbf0196
https://github.com/cuny-academic-commons/cac/commit/76b91cf27a584b4fa883028b21dc8c26b7005ae6