-
Notifications
You must be signed in to change notification settings - Fork 50
Support fully qualified table names in all statement types #270
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
Conversation
0f527dd to
35211d7
Compare
I'll probably learn that in a minute after reading the code, but at the moment I'm confused what does "fixing" mean, as in what's broken and what's different with this PR. Let's spell it out explicitly for when we browse the commit log in the future. |
tests/WP_SQLite_Driver_Tests.php
Outdated
| public function getInformationSchemaIsReadonlyWithUseTestData(): array { | ||
| return array( | ||
| array( 'INSERT INTO tables (table_name) VALUES ("t")' ), | ||
| array( 'REPLACE INTOtables (table_name) VALUES ("t")' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
array( 'REPLACE INTOtables (table_name) VALUES ("t")' ),
Is INTOtables intentional? If not, should we be worried that the tests pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel Oh, thanks for catching this! We should be worried indeed; seems like the lexer doesn't require the space in this case, but in MySQL, it should fail: https://onecompiler.com/mysql/442dtf2yu
I will create a ticket for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel Ah, no, all good, the INTO keyword is optional, so it parses INTOtables as an identifier (= table name), but since it targets the information schema, the correct exception is thrown.
The grammar:
REPLACE_SYMBOL (LOW_PRIORITY_SYMBOL | DELAYED_SYMBOL)? INTO_SYMBOL? ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, TIL
|
I'm still lost. Does it allow us to still use the fully-qualified table names, e.g. |
@adamziel Oh, sorry for not explaining this better in the description. This ensures and tests support for the USE information_schema;
SELECT * FROM tables; -- this targets information_schema.tables;
SELECT * FROM wp.wp_posts; -- this was not working for many statement types before
CREATE TABLE wp.new_table ...; -- phpMyAdmin does exactly this
-- etc.
USE wp;
SELECT * FROM infromation_schema.tables; -- this was already working |
827bb7a to
c7a06a6
Compare
|
@adamziel I rebased this to resolve conflicts. |
| * UPDATE t, information_schema.columns c SET t.column = c.column ... | ||
| */ | ||
| foreach ( $table_alias_map as $alias => $data ) { | ||
| if ( 'information_schema' === strtolower( $data['database'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely! Perhaps the corner case won't ever come up 🤞
adamziel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
This PR:
wp.my_table) for all statement types (they weren't supported before):Point 1 is needed for phpMyAdmin support, and point 2 naturally emerges from implementing 1.