Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement WP_Filesystem API for writing/reading files #27

Open
roborourke opened this issue Jan 10, 2013 · 8 comments
Open

Implement WP_Filesystem API for writing/reading files #27

roborourke opened this issue Jan 10, 2013 · 8 comments

Comments

@roborourke
Copy link
Owner

This is aiming to make the plugin more embeddable in themes, it will have to use the filesystem API if it's to be able to pass the theme checker for hosting on wp.org. Also it makes for better security.

Of course it also depends on lessphp itself so may be a no go.

@willmot
Copy link
Collaborator

willmot commented Jan 10, 2013

The problem with WP_Filesystem for something like generating the cache files is it will only work if direct writing is possible as FTP/SFTP/FTPS details aren't stored and you probably can't prompt for them every time the cache file needs writing.

This is made worse by the fact that WP_Filesystem only allows direct writing in specific circumstances, even though php does have is_writable access.

Thats the reason I'm not using it in BackUpWordPress for writing the backup files.

You could obviously work round it by prompting people to define their FTP/SFTP/FTPS details in wp-config.php.

@roborourke
Copy link
Owner Author

Cheers Tom, I realised this is a total no go as the lessc class uses file_put_contents etc... Best keep this is a plugin and go the theme dependency route.

@franz-josef-kaiser
Copy link
Collaborator

Maybe we should update the README with this info...

@roborourke
Copy link
Owner Author

I'm reopening this one for discussion as I want to try using the WP_Filesystem_Direct class (extends WP_Filesystem_Base).

It would be easy enough to test if it works at which point we could use that to take advantage of the automatic chmod'ing it does for security reasons.

One other reason for reconsidering is folks using origin-push CDN setups. Although for that particular case the best bet would be to add a post-compile action hook with the URLs and paths etc...

@roborourke roborourke reopened this Jan 21, 2013
@Rarst
Copy link
Contributor

Rarst commented Jan 21, 2013

I don't see much benefit other than chmod (which is to-be-really-sure kind of a thing in this context), however no downside either. To be fancy can try to write properly with filesystem and fall back to direct instead of asking credentials.

The small issue is overriding filesystem write(s) (there seems to be total of one, but I hadn't looked hard) in lessc, but can subclass it for that.

@bearded-avenger
Copy link

I'm using this class in a theme, tied to the theme customization API to provide real time color automation. I'm at the end of it and bug checking before submitting to repo, and Theme Check is of course flagging the file_put_contents.

If there's any way we can utilize WP Filesystem API that would be rad.

@roborourke
Copy link
Owner Author

Hi Nick, I did look into it some time ago but the only use case so far is
to get a theme accepted by the wp.org repo which most users of the plugin
don't do (much). It might be possible to get them to make an exception in
this case, the problems raised by Tom and Rarst are quite significant
barriers to this approach and it's fundamental to the functioning of the
plugin.

You could ask on the theme reviewers mailing list if it's ok
http://lists.wordpress.org/mailman/listinfo/theme-reviewers

They make exceptions for the likes of pagelines so it's not outside the
realms of possibility.

@bearded-avenger
Copy link

Fair enough, and totally understandable. Thanks man, appreciate the response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants