kasparsd / minit

A WordPress plugin to combine CSS and Javascript files.
GNU General Public License v2.0
286 stars 46 forks source link

Use WP Filesystem API for writing the Minit files #61

Closed geniuspro closed 8 years ago

geniuspro commented 8 years ago

Hello. Thanks for theme awesome simple plugin. I've integrated your plugin our theme. but theme check shows me one warning for minit.php.

WARNING: file_put_contents was found in the file minit.php File operations should use the WP_Filesystem methods instead of direct PHP filesystem calls. Line 161: if ( ! file_put_contents( $combined_file_path, $done_imploded ) )

How can i fix it?

Thanks.

kasparsd commented 8 years ago

Hey @geniuspro! Thanks for reporting that -- it should be changed, indeed.

However, please note that embedding a plugin is considered a bad practice for various reasons -- mainly, users can't install and use the latest version of the same plugin from the repository because of namespace conflicts. Please don't do that.

sidati commented 8 years ago

Unfortunately this cannot be happens because sometimes WP File System may ask for credentials of FTP while Minit generate minified files when any user visit the website :cry:

ryanhellyer commented 8 years ago

I've tried implementing this before and failed. If anyone can figure it out, I'd love to hear how you do it.

[note: I mean in a consistent way which doesn't break many sites]

kasparsd commented 8 years ago

@ryanhellyer What issues did you run into? I assume it wouldn't work with FTP access that requires credentials.

ryanhellyer commented 8 years ago

It keeps throwing errors on various server setups. It just turfs up that annoying "Enter your FTP credentials" junk. I've never found a workaround for that.

The general logic I've been given for using the WP file API is to avoid security concerns relating to extremely poorly shared server environments where users from one environment can overwrite files on another environment when the file owner is set incorrectly. I know there were hosts using this sort of setup back when the file API was added to core, but I really doubt there are many hosts still doing this today, and quite frankly working around setups that poor seem a waste of time.

file_put_contents() is a perfectly valid solution in this situation IMHO. I don't see why WordPress needs to have it's own special way of doing exactly the same thing.

ryanhellyer commented 8 years ago

I thought I had a solution for this back in 2014, but it turned out after publishing it, that my solution was complete crap. At that point I just gave up and used file_put_contents() ever since.

https://geek.hellyer.kiwi/2014/06/27/saving-css-files-uploads-folder/

szepeviktor commented 8 years ago

There are environments with virtual filesystems. For example GAE.

They have their own Filesystem API implementation.

ryanhellyer commented 8 years ago

@szepeviktor, but how doe that relate to this ticket? Wouldn't file_put_contents() work with them automatically anyway?

szepeviktor commented 8 years ago

@ryanhellyer Those fs-s need a Filesystem API implementation. For example https://wordpress.org/plugins/google-app-engine/

szepeviktor commented 8 years ago

Sorry, bad example. https://github.com/WordPress/WordPress/blob/master/wp-admin/includes/class-wp-filesystem-ssh2.php file_put_contents() does not work on this.

kasparsd commented 8 years ago

I think we've had a great discussion about the problems with using WP Filesystem and I suggest we keep using file_put_contents() and close this.