Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no-op encode/decode fixes #160 #161

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

Conversation

doriantaylor
Copy link
Contributor

#160 fixed for PG, tests pass over here.

@kasei
Copy link
Owner

kasei commented Mar 2, 2019

This looks mostly fine to me, but I don't see anything failing on my Pg setup using the current code in HEAD. Could you add tests that are fixed by this PR? Or describe in more detail something I should be seeing failing with the existing code? Thanks!

@doriantaylor
Copy link
Contributor Author

I was using Store::DBI against Postgres and typed in something with an acute accent into a web app and it came back out mangled. The entry also showed up in psql mangled alongside un-mangled entries from a different database, so that's how I knew it wasn't just a display problem.

First I tried commenting out the encode/decode calls and the problem went away. Then I looked at the DBD::Pg docs and the Postgres docs and concluded it was already handling the character sets automatically. I can't speak for MySQL or SQLite cause I was already three hours into this.

As for the test, it looks like all the store tests are centralized, so I didn't want to chance breaking tests for the other drivers. One weird note however: accent-y test content also showed up in psql all mangled, but the test passed. So maybe the test itself has been returning false positives? Again, I was already three hours into a solution and didn't want to open a Pandora's box.

@kasei
Copy link
Owner

kasei commented Mar 3, 2019

Understood. If the current code is broken (which it sounds like it is), then Pandora's box needs to be opened! :)

I'll look into writing some tests that expose this problem before merging the code.

Thanks!

@kasei
Copy link
Owner

kasei commented Mar 5, 2019

I see the encoding problems, but I'm afraid I don't know enough about how modern Pg and DBD::Pg work regarding unicode. My attempt at tests for this show what looks like good utf8 encoded bytes coming back from the database, but they're not marked as utf8 strings in perl, so it just ends up looking like a collection of bytes. I'll need to dig into this further to figure out what I'm missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants