Closed paulhennell closed 2 years ago
Actually, the code is dated, I have refactored the function to be simpler. Let me know if it works for you https://github.com/lesterchan/wp-dbmanager/archive/refs/heads/master.zip, and I will release it.
That's amazing and looks a lot cleaner!
I've tested by running wp cron event run dbmanager_cron_backup
under a user without permissions (which gives the error in 2.80.8) and got the following output:
PHP Notice: Error: file /{...}/wp-content/wp-cache-config.php is not writable. in /{...}/wp-content/plugins/wp-super-cache/wp-cache-phase2.php on line 1328
Notice: Error: file /{...}/wp-content/wp-cache-config.php is not writable. in /{...}/wp-content/plugins/wp-super-cache/wp-cache-phase2.php on line 1328
sh: 1: cannot create /{...}/backups/1667470677_-_db_name.sql.gz: Permission denied
mysqldump: [Warning] Using a password on the command line interface can be insecure.
mysqldump: Error: 'Access denied; you need (at least one of) the PROCESS privilege(s) for this operation' when trying to dump tablespaces
mysqldump: Got errno 0 on write
PHP Warning: md5_file(/{...}/backups/1667470677_-_db_name.sql.gz): failed to open stream: Permission denied in /{...}/wp-content/plugins/wp-dbmanager-master/wp-dbmanager.php on line 105
Warning: md5_file(/{...}/backups/1667470677_-_db_name.sql.gz): failed to open stream: Permission denied in /{...}/wp-content/plugins/wp-dbmanager-master/wp-dbmanager.php on line 105
PHP Warning: rename(/{...}/backups/1667470677_-_db_name.sql.gz,/{...}/backups/_-_1667470677_-_db_name.sql.gz): Permission denied in /{...}/wp-content/plugins/wp-dbmanager-master/wp-dbmanager.php on line 106
Warning: rename(/{...}/backups/1667470677_-_db_name.sql.gz,/{...}/backups/_-_1667470677_-_db_name.sql.gz): Permission denied in /{...}/wp-content/plugins/wp-dbmanager-master/wp-dbmanager.php on line 106
Executed the cron event 'dbmanager_cron_backup' in 0.179s.
Success: Executed a total of 1 cron event.db_name
So it no longer causes an endless loop and it throws much clearer 'permission errors' which makes fixing the issue easier too if you see this in logs. 👍
I feel like it should probably 'short circuit' earlier than it does - it seems to still try writing and renaming after establishing the folder isn't useable, but I'll let you assess the feasibility of that change.
It also seems wrong to end with the "Success" message, I'm not clear if that's a wp cli thing, or the end status of the plugin, but if there's a way to tap into a wp-cron failure status that would be nice so it's clear things are broken.
If there's nothing like that (or it's just wp cli) however, the errors here are much clearer than before and don't run until a timeout, so the real problems are fixed by the update 😄
Actually you are right, I am not solving the root of the problem. I think this fixed should do it https://github.com/lesterchan/wp-dbmanager/commit/46ac9c14bc99e48a98436a7d96872c87e5a41630
Can you help me test it again https://github.com/lesterchan/wp-dbmanager/archive/refs/heads/master.zip 🙏
That seems to have avoided the loop and removed all the errors.
Executed the cron event 'dbmanager_cron_backup' in 0.026s.
Success: Executed a total of 1 cron event.
Given there's quite a lot of warnings in the ui that the folder isn't writable I think that works to ensure it's not doing a load of stuff repeatedly it can't do, and it's clear there's an issue on the plugin info 😄
Thanks for looking at this Lester!
Thank you @paulhennell for helping me test!
I have released 2.80.9 =)
I'm not sure if this problem is what broke wp-cron, or just only uncovered when trying to fix a broken wp-cron - but it seems wp-dbmanager doesn't handle folders it can't access very well.
Running
wp cron event run --due-now
to fix a broken cron got me endless loops of:I disabled wp-dbmanager and tried again, which gave me the alert that I couldn't write to specific folders. Turns out I was running as the wrong user, and with the right user all was good.
Looked at the code anyway, and see that the lines around 392 in wp-dbmanager don't check for the false returned by opendir if
$folder
doesn't exist or is banned by permissions.I could do a PR to check it in place, but think the check should probably be at a higher level as
is_emtpy_folder
seems inappropriate for what should be 'is_writeable' or something.