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

Add a new method SQLite3Stmt::getSQL returning the SQL query used in the statement #3669

Closed
wants to merge 1 commit into from

Conversation

bohwaz
Copy link
Contributor

@bohwaz bohwaz commented Nov 20, 2018

::getSQL(true) will return the expanded SQL query (binded parameters replaced, "as" the query will be executed by SQL, eg. SELECT * FROM table WHERE a = 42;) but is only available with SQLite >= 3.14 (if true parameter is passed with an old version of SQLite the method will return an error)

while ::getSQL(false) (also the default) will just return the SQL query with the binded parameters as-is (eg. SELECT * FROM table WHERE a = :param;)

@cmb69
Copy link
Contributor

cmb69 commented Nov 21, 2018

@bohwaz I'd prefer to separate factoring out php_sqlite3_bind_params() from the introduction of SQLite3Stmt::getSQL() (i.e. two separate pull requests). While I like both, it seems that the former is a no-brainer, but the latter might deserve further discussion.

@bohwaz
Copy link
Contributor Author

bohwaz commented Nov 22, 2018

Sure will do that when I have some time.

return;
}

SQLITE3_CHECK_INITIALIZED(stmt_obj->db_obj, stmt_obj->initialised, SQLite3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong (as in several other places of the existing code as well). We' re mixing the initialization check of SQLite3 and SQLite3Stmt here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copy-pasted existing code here yeah, what would you suggest instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure (yet) what's the proper way to do it. Frankly, I'd rather wish that we didn't have to do these initialization checks at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Just tell me if I need to change anything to my patch :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think it's fine to leave this for now, and to review all those checks later. :)

@bohwaz
Copy link
Contributor Author

bohwaz commented Dec 7, 2018

@bohwaz I'd prefer to separate factoring out php_sqlite3_bind_params() from the introduction of SQLite3Stmt::getSQL() (i.e. two separate pull requests). While I like both, it seems that the former is a no-brainer, but the latter might deserve further discussion.

@cmb69 Done: see #3702

That PR has now been merged into this one.

#endif
}
else
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this file (mostly) uses K&R style, so I think it's best to stick with it. I.e. write the 3 lines above as a single line:

} else {

@cmb69
Copy link
Contributor

cmb69 commented Dec 7, 2018

Since PR #3702 has been applied, this PR has conflicts now. Could you please rebase?

Generally, I'm fine with the addition of this method. Thanks!

@bohwaz
Copy link
Contributor Author

bohwaz commented Dec 7, 2018

Yup, rebased, also resolved the else indentation issue, thanks.

Is there some kind of codesniffer for PHP C code so that I avoid that in the future?

@cmb69
Copy link
Contributor

cmb69 commented Dec 7, 2018

Is there some kind of codesniffer for PHP C code so that I avoid that in the future?

No, unfortunately not (yet).

If there are no objections, I'll apply this PR on the next weekend (i.e. in a week).

@cmb69
Copy link
Contributor

cmb69 commented Dec 15, 2018

Thanks, @bohwaz! Applied as 82af24f. Note that I have unindented the preprocessor instructions according to our coding standards (missed this earlier).

PS: Documented.

@cmb69 cmb69 closed this Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants