Update to 1.4.3.x?

Patch looks good. Thanks. Not committing this quite yet, because it would require updating to 1.4.3.x. Can anyone report on successes or failures with 1.4.3.x as compared to 1.4.2? Solving the Chrome 11 problems is great, but are any other side effects introduced with the new library? We should do this soon, but want to make sure there's been adequate testing. Thanks to anyone who comments.

@effulgentsia: I wrote a patch for the 6.x branch that detects which version of the library is installed and therefore works fine with 1.4.2 and 1.4.3. Feel free to steal my code and make it better :)

http://drupal.org/node/1124476#comment-4413202

This patch attempts to be more of a catch-all solution and should work with version 1.4.3 and earlier, also with the hybrid(!) directory structure you get if you checkout the src directly from github (for whatever crazy reason you might want to do that :-P)

I'm attaching a different version of this patch that can be applied on top of the patch at http://drupal.org/node/902410#comment-4418454

oops, here's the patch

I'm using 1.4.3.x and it works absolutely fine. I've been using a homegrown patch somewhat like katbaileys, but that shouldn't be a problem.

#4/#6 are clever, but I don't like adding unnecessary file_exists() calls to every page request, as in some hosting environments, disk IO is what kills performance and scalability. This is somewhat mitigated by only being for pages containing a plupload widget, but it's still bad practice, IMO.

I'm thinking #1 is still correct, but will hold off a little bit longer on some more testing of 1.4.3.2.

However, if anyone has a use-case for more flexible toggling between 1.4.2, 1.4.3, and github source, you can create a separate module with a hook_library_alter() implementation that takes the #6 approach. [EDIT: Or, submit a patch here that caches the discovery of where the files are, so that file_exists() only needs to run after a cache clear.]

It seems that the results from file_exists() are cached, but I'm not entirely sure what the lifetime of that cache is. It could be that the cache is the equivalent of a static variable that only lasts for one execution, or it could be stored more persistently in long-lived Apache processes.

If it's the latter, then I think it's safe to go with the file_exists() approach.

The other consideration to think about is that while some shared hosts are indeed file I/O bound, the Linux kernel does a really good job of caching things like the result from a stat() call with any free RAM available. More often than not, file_exists() may never even touch the disk. Conversely, using a persistent Drupal cache will introduce the addition of one extra MySQL query to every plupload page load. MySQL is often more of a bottleneck (compared to file I/O) in shared hosting, often because the MySQL query must traverse the network to a separate MySQL host -- I really don't think you're going to either gain or lose by preferring one method over the other.

Just my $.02.

the PHP stat cache is persistent only for that single script execution. That being said, it is entirely possible that in some past php version that cache could inherit/persist when using mod_php (would not ever occur when using a cgi version of php).

That being said, clearstatcache() is your friend.

Also good to note that the results of file_exists() are not cached if the result is null.

I would agree that caching this data in mysql would be less performant under any circumstance. If you did want some semi-persistent stand-alone cache then I'd consider storing this info in something like memcache or redis. If you are using APC and mod_php, then you could store it there too as it exposes a key-value store (but it is only shared between processes when using mod_php).

For now, I decided to commit #1, and updated README.txt accordingly. Will add a follow-up comment to this issue later today.

Sorry. I had to get on a call as I was writing #11. Anyway, based on various people here and on http://www.plupload.com/punbb/viewtopic.php?pid=3583 reporting happiness with 1.4.3.2, and my own testing not finding any problems, I decided #1 was good to go. Since people currently on 1.4.2 will be required to upgrade the library accordingly, I also committed #902410: plupload_library() requires plupload library to be in module folder rather than flexible or sites/all/libraries to start following best practice for where to place it.

I'm making the assumption that anyone currently using D7 is likely to want to use up to date libraries anyway, so there's not a compelling reason to retain BC for people to stay on 1.4.2. @justintime may come to a different conclusion for the D6 version in #1124476: plupload library versions >= 1.4.3 change file paths.

Thanks for the comments in #9/#10. I agree that a simple cache_get() is probably not the best approach, especially for people on shared hosting where non-db cache bins are not an option. But I also don't trust file stat caches (see #752730-11: Remove file_exists() during bootstrap). If someone has a use-case for needing the flexibility of supporting 1.4.2 or github source, please feel free to submit patches that explore other approaches (a config variable?) in this issue, or as a new issue.