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

Escape identifiers in metadata SQL properly when used as string literal #2442

Closed
wants to merge 1 commit into from

Conversation

deeky666
Copy link
Member

@deeky666 deeky666 commented Jul 7, 2016

Closes #2436

@Ocramius I tried to write tests but neither of my approaches felt right. First I tried to assertContains() the resulting SQL but that is not applicable to all platforms and methods as the given identifiers to escape are not always used as string literals (heavily depends on the platform's SQL statement) or are not even used at all. Then I tried to write functional tests but that is problematic, too as I cannot test the $database identifiers properly on all platforms because that involves creating a database (which is heavy and not supported on all platforms) with escape characters in the name.

Any idea what test to write here or should we take it "as is"?

@kimhemsoe
Copy link
Member

I agree that it is a bug, but this very much looks like a BC break for me. If i remember correct i even wrote a few scripts many years ago where i hit this and worked around it by calling quote manually. If ive have done that possible others have as well.

@deeky666
Copy link
Member Author

deeky666 commented Jul 7, 2016

@kimhemsoe yeah but it is wrong to do a quoteStringLiteral() for an identifier before passing it to the getList*SQL() methods because whether or not the identifier is treated as string literal is implementation detail and platform specific. The user should not know how the underlying SQL is built. Also the platform API does not make any statements about whether a given identifier should be quoted/escaped or the like. So I don't think this is a BC break but rather a misuse of the API then. It would be wrong if the (mis)usage of our API prevents us from fixing bugs.

@Ocramius
Copy link
Member

Ocramius commented Jul 7, 2016

@deeky666 I don't think that there is a sensible way to test this. I'd still apply some minimal assertions around the identifiers inside the SQL though, as that would show BC breaks in the quoting immediately, even though the tests may seem fragile.

@deeky666
Copy link
Member Author

@Ocramius done.

@Ocramius Ocramius closed this in b6b247a Jul 15, 2016
@Ocramius Ocramius self-assigned this Jul 15, 2016
Ocramius added a commit that referenced this pull request Jul 15, 2016
@Ocramius
Copy link
Member

Merged!

master: b6b247a
2.5: 4bca226

@deeky666
Copy link
Member Author

@Ocramius thanks

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