search

dizda / CloudBackupBundle

Be able to backup your database(s) and upload it to the cloud (Dropbox, Amazon S3, GoogleDrive, etc.)
MIT License
196 stars 57 forks source link

Started to write some logs & fixed the namespaces #55

Closed Nyholm closed 9 years ago

Nyholm commented 9 years ago

The namespaces should be in singular.

Logs

How should we do with the logs? Either we let each client/database worker be in charge of writing logs. Or we may introduce [Client|Database|Processor]Interface::getName() and have the BackupManager handle writing the logs.

Ref #51

dizda commented 9 years ago

Hi @Nyholm,

The namespaces should be in singular => Agree.

About the logs, indeed there are many ways to handle it. Right now as you can see, each Databases write themselves the dumping logs (cf. https://github.com/dizda/CloudBackupBundle/blob/feature/refactoring/Databases/BaseDatabase.php#L78) with the class name eg. Dumping MySQL database.

I'm not sure if this is the best way to do it. The getName() method can provide directly better names especially for clients & processors, because they don't have suffixe.

Or, we can handle it into the Chains, we inject monolog in DatabaseChain & ClientChain, then write logs in the foreach.. Well I don't know what decision to take.

What do you suggest?

Nyholm commented 9 years ago

Yeah. We could handle it in the Chain. But I want the Chains to have the same API as the other Clients/Databases. Ie, the ProcessorManager should not care if it handles a Chain or not.

Anyhow, I think we agree that the client should not handle the logging. I'll add getName in the Interface and see if I can figure something out.

dizda commented 9 years ago

Yes I think this is the most appropriate way.

Nyholm commented 9 years ago

This is not perfect. I would like then to show:

Dumping MySQL... Dumping MongoDB

I think we have to give up my thought of treating the chains as any other client/database and treat the Chain as Manager. We could easily delegate the logging to a DatabaseManager or a ClientManager.

dizda commented 9 years ago

Hmm I thought that you inject monolog into [Database|Client]Chain, to let it to handle writing in the foreach to get

Dumping MySQL...
Dumping MongoDB...
etc.
Nyholm commented 9 years ago

Yes, exactly. But that is not "treating the ClientChain like a normal Client". I think that is the best solution. I suggest to rename ClientChain to ClientManager and make sure that BackupManager always have an instance of the ClientManager.

dizda commented 9 years ago

Right, the client chain is not a client, it just contains the collection of clients.

dizda commented 9 years ago

Indeed, renaming it to ClientManager can be more clear.

Nyholm commented 9 years ago

There you go

dizda commented 9 years ago

Awesome PR, thank you bro'!

Nyholm commented 9 years ago

You are welcome. Thank you for merging.