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

Fix DBALStorage to not ignore exceptions#72

Open
enumag wants to merge 1 commit into
doctrine:masterfrom
enumag:patch-1
Open

Fix DBALStorage to not ignore exceptions#72
enumag wants to merge 1 commit into
doctrine:masterfrom
enumag:patch-1

Conversation

@enumag

@enumag enumag commented Jan 4, 2016

Copy link
Copy Markdown

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

Ocramius commented Jan 4, 2016

Copy link
Copy Markdown
Member

Requires testing. /cc @EmanueleMinotto

@EmanueleMinotto

Copy link
Copy Markdown
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

enumag commented Jan 4, 2016

Copy link
Copy Markdown
Author

@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
Copy Markdown
Member

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

@enumag

enumag commented Jan 4, 2016

Copy link
Copy Markdown
Author

@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
Copy Markdown
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

enumag commented Jan 4, 2016

Copy link
Copy Markdown
Author

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

@enumag

enumag commented Jan 4, 2016

Copy link
Copy Markdown
Author

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

@enumag

enumag commented Mar 15, 2016

Copy link
Copy Markdown
Author

@EmanueleMinotto Any progress?

@EmanueleMinotto

Copy link
Copy Markdown
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