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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants