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

Remove folder permissions notices #833

Merged
merged 33 commits into from
Mar 23, 2017
Merged

Remove folder permissions notices #833

merged 33 commits into from
Mar 23, 2017

Conversation

pdewouters
Copy link
Contributor

The newly added Path class is smart enough to create the backups folder either under wp-content or under the uploads folder which will always be writable by WordPress.

The whoami command is sometimes disabled on shared hosts. This can cause unnecessary errors for users.

I don't think we should use WP_Filesystem to create folders / files as the backup functionality is mostly run without user interaction, so displaying a login form doesn't help.

wp_mkdir_p should suffice, we should probably throw an error and bubble it up to UI if it is unsuccessful though.

https://github.com/humanmade/backupwordpress/blob/master/classes/class-path.php#L211

@pdewouters
Copy link
Contributor Author

@willmot any thoughts?

@willmot
Copy link
Contributor

willmot commented Jul 17, 2015

I agree we should remove those error messages, the advice to run chmod and chown is completely useless to most users and bad practise anyway.

However we'll still have an issue if the backups directory can't be created (which is the only time users see those above messages anyway).

I would see us using WP_Filesystem as part of some kind of setup screen or maybe from an action shown in the error message, IE it's a one time thing which the user initiates to use WP_Filesystem to create the backups folder with the correct permissions.

@willmot
Copy link
Contributor

willmot commented Jul 24, 2015

Discussed this in Slack:

We should:

  • Try to create the folder in WP_CONTENT using wp_mkdir_p
  • If not possible, attempt to use WP_Filesystem to create it (with user entering their details)
  • If not possible (or we need a fallback and can't ask user for details) then fallback to uploads
  • We'll still need notices for when the backups directory can't be created.

We'll probably want to tackle #538 at the same time

@pdewouters pdewouters assigned pdewouters and unassigned pdewouters Jul 29, 2015
Paul de Wouters added 5 commits July 31, 2015 09:19
# Conflicts:
#	classes/class-requirement.php
Also fixes duplicate notices
This allows us to get a list of possible paths for the backups folder without attempting to create them
@pdewouters
Copy link
Contributor Author

Fixes #855
Fixes #819

@pdewouters
Copy link
Contributor Author

@willmot do you think this is a good approach?

@willmot
Copy link
Contributor

willmot commented Jul 31, 2015

@pdewouters will need to check it out locally, could you drop in some screens of the user flow.

@pdewouters
Copy link
Contributor Author

screenshot 2015-09-04 10 48 53

To replicate, you'll need to chmod 000 /backups-folder but you can't do that from inside vagrant.
It will then display the FTP creds form until it gets correct credentials, then it will create the folder.

@pdewouters
Copy link
Contributor Author

  • hide everything else on page and explanation message

@willmot
Copy link
Contributor

willmot commented Sep 25, 2015

More like:

screen shot 2015-09-25 at 15 02 57

See #872 for the page heading "BackUpWordPress"

@willmot willmot removed their assignment Sep 25, 2015
@dashaluna
Copy link
Contributor

@willmot I'm adding it to the main wp-config.php file.

Can you confirm that wp-config.php is definitely the one being used (just try deleting it's contents or something). Maybe there's a wp-config.php in src.

Face palm it is indeed the src/wp-config.php file, I didn't notice it was there..

@dashaluna
Copy link
Contributor

dashaluna commented Sep 22, 2016

@willmot you were right, I had to edit src/wp-config.php file. I've tested the Run now functionality and updated this comment #833 (comment) with the test results.

It doesn't seem to work on VMWareFusion server at all. I've no idea why :( Can we ask someone else to test this on VMWareFusion? Or should we look at my instance of it and figure out why it's not saving backups?

@willmot
Copy link
Contributor

willmot commented Sep 27, 2016

I'm running Fusion locally and can confirm it's working fine. Do the unit tests run when using vmware for you?

@willmot
Copy link
Contributor

willmot commented Sep 27, 2016

@dashaluna I went through and retested the whole flow here and everything is working as expected. Shall we screenshare to debug your Fusion issues?

@willmot
Copy link
Contributor

willmot commented Nov 3, 2016

@sambulance @shadyvb this one just needs conflict resolving and can then be merged.

@sambulance
Copy link
Contributor

Part of the merge conflict was because we removed the is_backup_possible() check from around the admin recently (#1030). I've created a segmented version of this to just check if we have access to read and write directories, and if not, show the credentials form.

@sambulance
Copy link
Contributor

@shadyvb Please can you review/merge this, specifically my last commit?

@katmoody
Copy link
Contributor

@sambulance is this completed?

@sambulance
Copy link
Contributor

This is waiting on code review, doesn't have to be Shady.

@katmoody
Copy link
Contributor

katmoody commented Mar 23, 2017

I think this is still looking for a review per Sam's last response. @willmot or @pdewouters since you were the most active in earlier parts of this one, can you give it a look when you're able and see if we're able to go forward on this with the next release or if there are any other specific tasks that need to be completed first? Thanks!

@willmot willmot merged commit 9901a7f into master Mar 23, 2017
@willmot willmot deleted the remove-whoami branch March 23, 2017 16:33
@willmot
Copy link
Contributor

willmot commented Mar 23, 2017

Yep this looks good 👍

@katmoody katmoody added this to the 3.6.4 milestone Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants