Coding style proposal: double quotes for SQL

Coding style proposal: double quotes for SQL

av David Mudrák -
Antall svar: 14
Bilde av Core developers Bilde av Documentation writers Bilde av Moodle HQ Bilde av Particularly helpful Moodlers Bilde av Peer reviewers Bilde av Plugin developers Bilde av Plugins guardians Bilde av Testers Bilde av Translators
This is a proposal for adding the following rule into the Development:Coding_style.

Use double quotes for SQL statements, database table names and database field names. This does not influence functionality but with a syntax highlighting properly set, this makes the code more readable.

See the attached screenshot for an example of such highlighting.

Vedlegg sql-highlight.png
Gjennomsnittlig vurdering: -
Som svar til David Mudrák

Re: Coding style proposal: double quotes for SQL

av Ravishankar Somasundaram -
David ++


I too have felt the same , and have been coding till now in this manner,
it also makes things easier for me, instead of starting a single quote and then double quote inside for making interpolation of variables,i just use a double quote at the start and then end it with the same,many consider that as a bad practice but no body is justifying why.

This will work for single level variable transformations ex:

echo " some text $textdesc , end of text ";


Som svar til David Mudrák

Re: Coding style proposal: double quotes for SQL

av Aaron Barnes -
Hi David,

Moodle 2 is making the switch to prepared statements - using placeholders for data in SQL statements.

This means you will no longer have to frequently append values and double quotes would be superfluous.

Here is some more info about the new database layer:
http://docs.moodle.org/en/Development:DB_layer_2.0

Cheers,
Aaron
Som svar til Aaron Barnes

Re: Coding style proposal: double quotes for SQL

av David Mudrák -
Bilde av Core developers Bilde av Documentation writers Bilde av Moodle HQ Bilde av Particularly helpful Moodlers Bilde av Peer reviewers Bilde av Plugin developers Bilde av Plugins guardians Bilde av Testers Bilde av Translators

Aaron, thanks for the comment.

Yes, I know the new DB layer and I know how the SQL code looks like in 2.0. Also, using variables inside SQL statement has nothing to do with prepared statements but with binding parameters (syntax is the same but the concept is different). Still, there are situations where you use single quotes (') inside statements - to quote static strings, for example. And, there is a quite often case for putting variables into statements, typically the result of $DB->get_in_or_equal() call.

But apparently I have not underlined the key point here. My proposal has nothing to do with variables expansion. The main reason why I proposed is:

  • Petr already uses this syntax for all SQL's he has touched in 2.0. So this is not new at all.
  • syntax highlighting - even this may appear unimportant, it really counts if you need to quickly find something.
This means that double quotes should be also used at places like $DB->get_records() parameters - again for quick spotting of any SQL stuff.


Som svar til David Mudrák

Re: Coding style proposal: double quotes for SQL

av Eloy Lafuente (stronk7) -
Bilde av Core developers Bilde av Documentation writers Bilde av Moodle HQ Bilde av Peer reviewers Bilde av Plugin developers Bilde av Testers
Hi David,

does the double/single quote make any difference really from a syntax highlight perspective?

I think I haven't anything special in my vim configuration causing both quoted strings to be displayed exactly the same.

Just note I'm using php_sql_query = 1 to highlight the SQL keywords differently within those strings but I get both perfectly pink (brown in your example).

In any case, I really think it's a matter of each IDE capabilities how those strings are displayed and that must not be the cause for any change in our coding style.

And, talking about coding... it used to be generally accepted to use single quotes when possible, afaik. It had some (minor I guess) parsing benefits and so on.

Ciao smiler
Vedlegg quotesandquotes.png
Som svar til Eloy Lafuente (stronk7)

Re: Coding style proposal: double quotes for SQL

av David Mudrák -
Bilde av Core developers Bilde av Documentation writers Bilde av Moodle HQ Bilde av Particularly helpful Moodlers Bilde av Peer reviewers Bilde av Plugin developers Bilde av Plugins guardians Bilde av Testers Bilde av Translators
The proposed solution is suitable for all IDEs that are able to distinguish between single quoted strings and double quoted strings. Anyway, this was just an idea. No press. In fact, this was originally Petr's idea who "forced" me to use this syntax during his review of the Workshop code...
Som svar til David Mudrák

Re: Coding style proposal: double quotes for SQL

av Dan Poltawski -
I really don't like this proposal, for three reasons:

1) I have slight obsessive compulsive disorder when seeing string literals in double quotes. The age old wisdom that, its faster and more memory efficient as php doesn't need to do complicated string parsing makes me feel like i'm boosting performance with every double quoted string converted to single quotes!

2) I don't think we should be modifying the coding guidelines to fix broken syntax highlighting! We surely should be patching the syntax highlighter to do the right thing!

3) In non-syntax highlighted world (or if you don't trust your syntax-highlighter) I think there are readability benefits to distinguishing string literals being properly distinguished.

For example: I'm doing a security audit for SQL injection attacks I have to look a lot more closely at double quoted string to examine the contents of the string deeply to check for variables which might sneak in. With single quoted I don't have to look so deeply..
Som svar til Dan Poltawski

Re: Coding style proposal: double quotes for SQL

av Tim Hunt -
Bilde av Core developers Bilde av Documentation writers Bilde av Particularly helpful Moodlers Bilde av Peer reviewers Bilde av Plugin developers
I agree with Dan. -1 for David's original proposal.
Som svar til Tim Hunt

Re: Coding style proposal: double quotes for SQL

av Petr Skoda -
Bilde av Core developers Bilde av Documentation writers Bilde av Peer reviewers Bilde av Plugin developers
I do not agree with Dan, +1 from me.

Majority of SQL code in 1.9 and HEAD is already in "double quotes". Making it consistent makes sense to me. Converting everything to ' does not seem to be an option here.

Petr
Som svar til Petr Skoda

Re: Coding style proposal: double quotes for SQL

av Mark Johnson -
Bilde av Core developers Bilde av Particularly helpful Moodlers Bilde av Peer reviewers Bilde av Plugin developers
Surely making it consistent should be the other way around? All non-SQL strings (and some SQL strings) use single quotes. In the context of the PHP what the string contains doesn't matter, so just because they contian SQL doesn't mean they should be represented differently.
I don't think the coding guidelines should be changed to allow for where they haven't been followed - the code should be brought into line.

Additionally, I don't think that syntax highlighting is a good enough reason for introducing a coding convention, since it's not universal. If you follow the convention of putting SQL keywords in uppercase, then SQL strings stand out amongst all other code, which according to the guidelines should be lowercase.

Som svar til Mark Johnson

Re: Coding style proposal: double quotes for SQL

av Mark Johnson -
Bilde av Core developers Bilde av Particularly helpful Moodlers Bilde av Peer reviewers Bilde av Plugin developers
An additional point - since {braces} in a double-quoted string are used for interpolation (is that the right word?), they could be a cause for confusion, as the new database API uses them to enclose table names. For example:

$sql = 'SELECT * FROM {user} WHERE id = 3;';
$sql = "SELECT * FROM {user} WHERE id = 3;";
Would behave the same, but
$sql = 'SELECT * FROM {$table->name} WHERE id = 3;';
$sql = "SELECT * FROM {$table->name} WHERE id = 3;";
wouldn't.

If I saw a double-quoted string containing something in braces I'd immediately assume it was being used for interpolation, and since all SQL strings are to contain them from now on where they AREN'T being used for interpolation I think double-quotes are a bad idea.
Gjennomsnittlig vurdering:Useful (1)
Som svar til Petr Skoda

Re: Coding style proposal: double quotes for SQL

av Penny Leach -
I agree with Dan and Tim.

I hate double quotes smiler And Mark's point about table placeholders {} makes a lot of sense as a reason to use single quotes as well.

(PS eloy thanks for the vim-tip ;) )
Som svar til Penny Leach

Re: Coding style proposal: double quotes for SQL

av Martín Langhoff -
My much-diluted vote comes in for Eloy, Dan, Tim, Penny and Mark.

Specially with the new SQL table syntax, and the use of placeholders, there is little reason to use doublequotes.

If we're to consolidate our usage of quotes in SQL, it'd make sense to consolidate on single quotes. This will make odd code immediately stand out.
Som svar til David Mudrák

Re: Coding style proposal: double quotes for SQL

av Petr Skoda -
Bilde av Core developers Bilde av Documentation writers Bilde av Peer reviewers Bilde av Plugin developers
Great, we have a winner - the single quotes stort smil

I think we could wait a bit for the final word from MD and then convert all SQL to this new coding style, any objections? I hope not, patches can not merge due to previous changes anyway.

Petr
Som svar til Petr Skoda

Re: Coding style proposal: double quotes for SQL

av Martin Dougiamas -
Bilde av Core developers Bilde av Documentation writers Bilde av Moodle HQ Bilde av Particularly helpful Moodlers Bilde av Plugin developers Bilde av Testers
Shrug, I really don't think this is too important to standardise, even though I'd probbaly put single quotes in the coding guidelines for new code.

I'd leave old stuff as it is and let it get slowly cleaned up when people edit it.