Skip to content
This repository has been archived by the owner on Feb 5, 2024. It is now read-only.

Fix DBALStorage to not ignore exceptions #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

enumag
Copy link

@enumag enumag commented Jan 4, 2016

Is there some reason to catch all exceptions? If there is a problem with the storage I want to know about it, not silently ignore it.

@Ocramius
Copy link
Member

Ocramius commented Jan 4, 2016

Requires testing. /cc @EmanueleMinotto

@EmanueleMinotto
Copy link
Member

@enumag you're right, exceptions should be thrown there as well, anyway without tests a merge won't be safe (I'll cover them asap if you can't).

@enumag
Copy link
Author

enumag commented Jan 4, 2016

@EmanueleMinotto I don't have time for that now... Also I've noticed that the DBALStorage completely ignores the $storageName argument. Maybe it should be used as a prefix for the key?

Anyway I've duplicated the class to my project for the time being (without the try-catch blocks).

@EmanueleMinotto
Copy link
Member

@enumag yes, I'm reviewing everything in these days

@enumag
Copy link
Author

enumag commented Jan 4, 2016

@EmanueleMinotto I'm also wondering if there should be some sort of support for transactions (when inserting/updateing multiple values). Not sure if that's relevant to other storages than sql database.

@EmanueleMinotto
Copy link
Member

@enumag it would be great if you could open an issue for every thought :)
because $storageName seems a bug, I'm still not sure about transactions and no one of these is related to DBALStorage exceptions

@enumag
Copy link
Author

enumag commented Jan 4, 2016

Ok, I'll open separate issues. :-)

@enumag
Copy link
Author

enumag commented Jan 4, 2016

All done. You can delete the unrelated commens from this issue if you want.

@enumag
Copy link
Author

enumag commented Mar 15, 2016

@EmanueleMinotto Any progress?

@EmanueleMinotto
Copy link
Member

@enumag not yet, I'm busy with some things + work, I'll take a look asap

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants