junaidbhura / fly-dynamic-image-resizer

Fly Dynamic Image Resizer plugin for WordPress
https://wordpress.org/plugins/fly-dynamic-image-resizer/
MIT License
162 stars 26 forks source link

Useless getimagesize() in code? #21

Closed unknownskl closed 4 years ago

unknownskl commented 5 years ago

Hello,

I've been checking the code because we are having an issue regarding the plugin when using the plugin with images on a fileshare (EFS / Azure SMB). I've been trying to understand why the following line is needed when we already know the dimensions of the image?

Can you explain why there is an extra call to getimagesize()?

https://github.com/junaidbhura/fly-dynamic-image-resizer/blob/bdfc156c9aeee83bdcf3f8f554824faff4ffcb17/inc/class-core.php#L257

Thanks

junaidbhura commented 5 years ago

Hey @unknownskl apologies for the super late response.

That's a good question. I'd added it there to resolve another issue, I think. I'm going to have to take a deeper look to see if we can improve this further.

Please feel free to submit a pull request in the mean time 😉

unknownskl commented 5 years ago

Hey @junaidbhura,

The pull request is still coming. The changes are committed to my forked version. The change is passing the tests, but i have no idea if it breaks some features. Once it is tested on the website, i will submit the pull request.

You can find the changes in this branch: https://github.com/unknownskl/fly-dynamic-image-resizer/tree/feature/speedup_nfs

junaidbhura commented 4 years ago

@unknownskl Apologies for the super late response!

I had some time to look into this today, and it looks like we actually do need the getimagesize function in there.

This is because users are able to create image sizes like array( 100, 9999 ) with a soft crop.

So if we didn't have the getimagesize in there, it would give us a size of 100 X 9999, which is incorrect.

I'm closing this issue for now, but I'm making a note of the performance hit for NFS / mounted filesystems. I'll see if I can address this in a future update.

Please feel free to continue this discussion here!