Restore command

[jongotlin]

Tried to implement a restore command requested in #106. Since we're dealing with multiple dbs, multiple clients, split files etc I think it will be hard (impossible) to implement restoring for all variations. The code itself is very wip but please give some feedback on the path forward. Or is it even something we shouldn't support due to limitations described above?

The code works with one db, Gaufrette (S3) and zip. It has also a dependency on mysql running on the server running the command.

[apsylone]

I could try to work on implementation for others DB and Client if you would like some helps. That's doesn't seems to be so hard to implement and there's no so much things to do :-)

[jongotlin]

@apsylone Thanks! That will be much appreciated.

[dizda]

Hey guys, thanks for making this incredible feature real!

Just one thing, I think the script should not decide to use the first provider to get the backup archive, but might be better to ask the user which one of the available providers he wants to use?

This should be quite easily doable with Symfony Interactive commands (cf. http://symfony.com/doc/current/components/console/helpers/questionhelper.html).

What do you guys think?

[jongotlin]

@dizda Sure it's a nice feature. But let's add that kind of features separately from this PR.

I'll continue setting up the interfaces and some error handling.

[Nyholm]

To get some clarity from my last review: I would like (and I think it is possible) to implement this feature without breaking BC. I do also feel it is important to comply with Liskov Substitution Principle (LSP).

Since adding methods to an interface breaks BC we need to create a new interface. So instead of ClientInterface::download I suggest something like DownloadableClientInterface::download. That interface should (naturally) only be put on classes that actually implement download(). Same with DatabaseInterface::restore and ProcessorInterface::getUncompressCommand, we should consider a RestoreableDatabaseInterface::restore and UncpmpressableProcessorInterface::getUncompressCommand.

The ClientManager::download() is fine because that is a normal class. That function should be implemented todo something like:

foreach ($this->children as $child) {
  if ($child instanceof DownloadableClient) {
    try {
      return $child->donwload()
    } catch (...) {
      // Error handling
   }
}

throw \Exception()

When we consider the ClientManager::addClient, that is fine as it is. It still accepts ClientInterface but now has support for different kinds of "special" clients. We have pushed the problem of methods not implemented to one single place and all ClientInterface and DownloadableClientInterface comply with LSP.

With that said.. I think this PR looks good. It is only a matter of adding some new interfaces.

[jongotlin]

@Nyholm Do you know why the tests fails on HHVM? Failed asserting that exception message '' contains 'No downloadable client is registered.'.

[Nyholm]

I was wondering that as well. I guess you can't Define the exception message as a protected property like that. (Not sure why...)

Try using a constructor.

[jongotlin]

Works, but looks ugly. What about MissingDownloadableClientsException::create()?

[Nyholm]

I like that. Or exceptions like these (if it makes sense).

Doing MissingDownloadableClientsException::create() will be great.

[jongotlin]

What do you think, do we also need the user to provide --force to run the restore command? Except for having to enable restoring (see my last commit).

[jongotlin]

@Nyholm; better safe than sorry!

[jongotlin]

Dropping WIP flag. Please review :-)

[apsylone]

Nice work done ! Thanks you for your job @jongotlin ! Please @Nyholm review ! :D Can't wait to use it for my own project...

[dizda]

Hey, could you rebase with master? There are some conflicts with Resources/config/services.yml. Thanks! :)

[jongotlin]

Rebased. But some memory issues with Travis I cannot look into right now.

[jongotlin]

Could not make Travis happy other than limiting number of Symfony versions in https://github.com/dizda/CloudBackupBundle/pull/110/commits/1e3f452fc93b88617850e60dea4760b7dc7ba969 Thanks @Nyholm

[Nyholm]

Sf 2.3 is maintained for 2 more month. Im okey with dropping 2.1 because of this.

[jongotlin]

Any plan to merge this?

[Nyholm]

Yes, I will have some time to test this PR this week. Code review looks good. I'll make sure to do a final review.

[Nyholm]

There was a minor bug with the mysql filename. I fixed that bug here: https://github.com/jongotlin/CloudBackupBundle/pull/1

I also added dropbox support.

[apsylone]

Is it possible to implement FlySystem as well ? Because Gaufrette is not so up to date with AWS S3...

[Nyholm]

That is a great idea. But it should be a separate PR. Let's get this merged first

[apsylone]

Any ETA on this ?

[akovalyov]

@Nyholm is there a possibility to get it merged soon? I can test this branch on my envs, if it would be helpful.

[Nyholm]

There are some minor comments I would like to be addressed before I can merge. When the PR is updated I will do my final review. If you want to help, please review/test it and try to find bugs so we can address them before we release this change.

[akovalyov]

@Nyholm OK, will try to, thank you!

[apsylone]

Hi @Nyholm , your comments has been taken in consideration by @jongotlin . Any chance to have it merged soon ? I've tried it on my side and it works like a charm :-)

[jongotlin]

Thanks @apsylone for testing it. :+1:

[Nyholm]

Thank you @apsylone for testing. It is great that there are more of us doing some test.

Great @jongotlin, I cannot see any more fixes that needs to be done. I like that you added tests and that this feature keeps BC.

[jongotlin]

Thanks for merging! May you tag a release as well? @Nyholm