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

fix for jsonb contains operator #1111

Closed
wants to merge 2 commits into from
Closed

fix for jsonb contains operator #1111

wants to merge 2 commits into from

Conversation

adrianrudnik
Copy link

Q A
Type bug
BC Break no
Fixed issues #1106

Summary

Looking at the current code, the executeQuery usage mentions some case with a REPAIR returning a result set. It's unclear to my why this is important when the result is never returned, requested or in any other way important to a migration, except its success.

Usage of a static statement, instead of a prepared one, allows for simple JSONB contain queries again, like in 3.0.1.

As for testing, I'm unsure how to do it and I could not find any good approach to simulate a DBAL executeQuery vs executeStatement that would lead to this error to make it testable.

@andrew-demb
Copy link

See comment, why the same change was reverted
#888 (comment)

@adrianrudnik
Copy link
Author

Do you consider REPAIR TABLE and OPTIMIZE TABLE worthy of migrations? They do nothing to the schema and representation of the data itself. If so, I have no clue how to migrate JSONs right now.

@goetas
Copy link
Member

goetas commented Jan 7, 2021

Well, this here is tricky, we are already going back and forth 3 times changing this line. Ideally it should be executeUpdate, but some vendors (mysql) have instructions that prefer the other way around.

(this because with executeQuery will never use PDO::exec, that does not try to parse ?)

IMO we should leave this as it is, and use https://wiki.php.net/rfc/pdo_escape_placeholders to "workaround" the issue.

@adrianrudnik
Copy link
Author

adrianrudnik commented Jan 7, 2021

Thanks @goetas , nice to know about the RFC for ??. It works for now but will break BC for PHP < 7.4 if I read the RFC correctly. Also the dev creating the migrations needs to be aware of it as you cant just build your SQL inside a query window and simply paste it around without further editing, and some IDE support goes bad because of the bad syntax, but at least it migrates.

A warning would be nice in that case but seems hard to trigger as Doctrine and then deep down PDO itself will interpret the query, not the migrations. Maybe handle the parts of the error message ERROR: syntax error at or near "$1" of Doctrine\DBAL\Exception\SyntaxErrorException and hint towards this situation / suggestions.

Also just rediscovered that you also could just $this->connection->executeStatement inside a migration if everything else fails.

Thanks for your work and time, I'll close this.

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.

3 participants