backdrop-contrib / backup_migrate

Port of the Drupal backup_migrate module to Backdrop
GNU General Public License v2.0
7 stars 14 forks source link

Bee Intregration #115

Closed rbargerhuff closed 10 months ago

rbargerhuff commented 12 months ago

My team and I strongly feel that the backup_migrate module needs Bee integration. My team and I are currently working on this at the time of submitting this issue. Once it is complete and it has been fully tested, we will submit a pull request.

argiepiano commented 11 months ago

Thank you! I noticed you just pushed a PR: https://github.com/backdrop-contrib/backup_migrate/pull/121

I'll check it soon.

rbargerhuff commented 11 months ago

I had to close and reopen the PR because some kind of fluke occurred during the initial check that takes place when a PR is submitted. The PR passed all checks on the second attempt. :)

argiepiano commented 11 months ago

@rbargerhuff, thanks again for providing the PR. I tested the "informational" options bam-created, bam-destinations, bam-profiles and bam-sources.

I'm running into troubles with bam-created, and getting this every time I run it:

Screen Shot 2023-12-10 at 9 18 11 AM

This doesn't show any issues in the UI:

Screen Shot 2023-12-10 at 9 23 56 AM

I also had issues with bam-sources the first time I ran it, but not on repeat running:

Screen Shot 2023-12-10 at 9 16 59 AM

I haven't yet tested the actual backup and restore options.

argiepiano commented 11 months ago

Hmm... interesting. When I added a second backup through the UI and tried bee bam-created, now it's working correctly. Will keep trying to see if I can reproduce this issue.

argiepiano commented 11 months ago

Actually, no, the warnings are still there for bam-created, even after adding more backups.

argiepiano commented 11 months ago

Tested bam-backup and bam-restore - they both work great!

@rbargerhuff I've added a comment in the PR.

Here are a couple of other comments:

  1. The description of the arguments for bam-restore is somewhat confusing:
Screen Shot 2023-12-10 at 9 45 01 AM

If you look at the second example, the first argument (db) could be better described as "Restore to" to match the UI (the description of this argument actually explains this, so this is a lesser issue).

However, the second argument in the description, destination is really confusing. This is actually the location of the file to be used for the restore. Is there a way to modify the description of that argument to something like "The id of location of the backup file".

  1. This is me trying to break things 😄 . I tried bee bee bb db download. Instead of the command downloading the database into a file, it dump the who binary into my terminal! Any way to either prevent this, or to make it work the way the UI works?

  2. So, when I restore a db, I get logged out as admin from my site. The UI doesn't cause this issue. Not sure if there is a way to prevent this? It may not be possible... if so, then perhaps add a warning "Restoring the DB will cause all users to be logged out".

Thanks again. I think this is great!

rbargerhuff commented 11 months ago

Hi Argie... I'm going to remove my recent update to my pull request and try to address some of these issues either today or tomorrow. I had no idea you responded until I relaunched Chrome and then I was able to see all your comments.

Update: Does not look like I can just remove the Pull Request easily. I apologize, I'm still new to these procedures.

argiepiano commented 11 months ago

Thanks!

@rbargerhuff, there is no need to remove the PR. Simply make your changes, commit them, and push them to the correct branch in your fork (rbargerhuff:rbargerhuff-#115-bee-integration). That will update the PR automatically.

rbargerhuff commented 11 months ago

Thank you @argiepiano . I am still looking at the issues you have encountered and have not been able to recreate them. Tomorrow I will try some other means to recreate the issues with my colleague.

I can say though, supporting the use of the download destination is outside the scope of this PR. I would need assistance with trying to support that.

argiepiano commented 11 months ago

Thanks @rbargerhuff. I'll try to look into why I got those warnings as well.

Re: download. That's perfectly fine! It may be helpful, however, to include some explanation in the help text about downloading not being supported. Does that sound OK?

rbargerhuff commented 11 months ago

Hi @argiepiano , the errors you were seeing were warnings that are shown in environments running PHP 8.x. Our system was on PHP 7.4.x which does not show those warnings. We switched our environment to PHP 8.x temporarily and were able recreate the warnings.

In order to prevent these warnings, we felt that includes/files.inc should be modified to better handle a scenario that allows error suppression to no longer be needed. We were able to accomplish this with a small code change that we carried out in 2 other locations within that file.

Please note, that there are other warnings that will be displayed when operating in a PHP 8.x environment. These warnings however, are outside the scope of this PR because they would be best handled when the module is updated for PHP 8.x compatibility.

All the issues and concerns that you have found that could be addressed have been addressed.

This is me trying to break things 😄 . I tried bee bee bb db download. Instead of the command downloading the database into a file, it dump the who binary into my terminal! Any way to either prevent this, or to make it work the way the UI works?

This was looked into and my colleague and I do not see how this can be implemented without further developing the module in order to support bee's integration with this. We decided it was outside the scope of this PR and instead we eliminated both download and upload destinations from being allowed.

So, when I restore a db, I get logged out as admin from my site. The UI doesn't cause this issue. Not sure if there is a way to prevent this? It may not be possible... if so, then perhaps add a warning "Restoring the DB will cause all users to be logged out".

We looked into this and noted that sessions are not supported in CLI environments. The session is being carried over by a combination of Backdrop core and the web browser. In fact there is code in Backdrop core that prevents sessions from being started if the environment is CLI. We added a warning that informs the user that restoring the database will result in all users being logged out.

argiepiano commented 10 months ago

Thanks @rbargerhuff. I've tested and it works great.

The problem of being logged out when restoring a db backup with bee needs to be looked into more closely. BAM (by default) doesn't include the sessions table in backed up database. When you restore from the UI, the old session table is somehow preserved intact, but restoring from bee wipes out the content. I'm not sure how and why this is happening. It may very well be what you are describing re: sessions being prevented when running bee. It's good that you added a warning. Perhaps we can look at this more closely in the future.

I will go ahead and merge this. Thank you so much for all the great work!