atom / language-php

PHP package for Atom
Other
119 stars 118 forks source link

Embedded SQL-Syntax only works for inline SQL #87

Open screeny05 opened 9 years ago

screeny05 commented 9 years ago

Used Atom Version: 1.0 Expected behaviour: Correct highlighting of SQL within PHP-Strings Has this ever worked: No

Highlighting of SQL within strings is a very useful feature, but there's a little problem:

the language-php package only seems to highlight them if the beginning of the sql-statement lies on the same line as the opening of the string with quotation marks.

image (Notice, that Lines 6-8 are all green)

zr9 commented 8 years ago

Atom uses textmate like grammar, if main principles of grammar is same then this one is impossible. Textmate grammar limited to 1 rule per line, so it not possible to match few lines with 1 regex, here is info about that behavior Textmate Grammar

That problem require a multiline matching of text, because it is not possible otherwise distinguish SQL string from regular string basing only on quote.

Someone can fill bug/question about grammar to core devs?

thisispiers commented 8 years ago

any updates? an issue where the string is incorrectly ended occurs with concatenated, embedded sql and it screws up the highlighting of the rest of the file

zr9 commented 8 years ago

@thisispiers You probably talking about other one, could you post test example which code broke highlight? That one is related only to case when you not get correct highlight if SQL is not single line.

Arcanemagus commented 8 years ago

This is the same issue as #52, the switch to SQL gets broken with any break in the string. (Including, apparently, a newline.)

50Wliu commented 8 years ago

@Arcanemagus I'm going to keep this one separate just in case.

zr9 commented 8 years ago

@Arcanemagus @50Wliu In concept of bugs this is 2 different issues, #52 can be solved. When this one almost nope. Since tokenizer not support multiline capture

50Wliu commented 8 years ago

Since tokenizer not support multiline capture

Yes it does: just use begin/end instead of match.

zr9 commented 8 years ago

@50Wliu Yup in global it does. Problem so being and end is match only edges, and in that case there will be no different between SQL inside quotes and Str inside quotes. For detect this case there is need proper multiline regex with checks what is between begin and end. So for example is not possible to make expression(as simple example) like /"\s*SELECT.*"/ because part .* not catch multiline.

The other way to do it, is do quotes support in grammar and all cases inside them, it works if i'm correctly recall atom and textmate grammar. But itself way take too much time and efforts and still can give false positive in that case.

The third way is to set begin and end to some uniq to SQL tokens, problem so SQL not have some special end token.

james-radford-854 commented 8 years ago

We could match /"\s*--\s*SQL$/, which would match the following:

$foo = "-- SQL
    SELECT *
    FROM users
    WHERE foo = 'bar'
";
zr9 commented 8 years ago

@james-radford-854 That is a bit crazy way to invent syntax to cover highlight i'm would say. Since in that case everyone need to agree on that way and keep it, which i'm assume not happens. But yup will work in total

PeteDevoy commented 8 years ago

I assume most people use PDO now and most people name their variables for SQL $sql, $query or $statement. So if the code could pattern match for ->exec(, ->prepare(, $sql =, etc. it might be an acceptable compromise?

̣Even if it's a bad idea because of false positives I would like to try for myself anyway. Please could someone tell me where in the file I would need to write the patterns? Disregard that, looks like there is no easy way to hack at the grammar files.

zr9 commented 8 years ago

@PeteDevoy It basically not hard, you need file /grammars/php.cson. And you need to use begin/end to capture multiline, end in your case probably will be /["'];/ and begin some like /$sql\s*=/. But please keep in mind it only examples, in reality you need to set them in synergy with others, because if i'm not wrong if you capture group which belongs to other then other will fall. May be wrong since used grammar a bit time ago. As reference look on string-double-quoted group, it almost close to perfect example i'm assume, since it have not only group capture but pattern include as well. And please keep in mind so your end syntax need too be a unique string, since if i'm not wrong it dumb capture, and so it captures without patterns check and then only start pattern checking.

PeteDevoy commented 8 years ago

@zr9 Yes, thanks the tips. What you are saying sounds perfectly correct but unfortunately I do not think /grammars/php.cson in file:///~/.atom/? I believe I would have to change at source and then do a custom build. Although it might be simple technically speaking, custom building Atom sounds like a it cause headaches :(

50Wliu commented 8 years ago

@PeteDevoy apm develop language-php will clone language-php to ~/github/language-php. Then you can start atom using atom --dev. Changes made to language-php will be reflected there (just reload the window).

zr9 commented 8 years ago

@PeteDevoy Yup, as @50Wliu said you can use source of package, it not require rebuilt whole atom source. If you not like apm for some reason you can directly make a clone on this repository in atom packages dir, it will also work.

adjenks commented 5 years ago

In response to @james-radford-854 who said this:

We could match /"\s*--\s*SQL$/, which would match the following:

$foo = "-- SQL
    SELECT *
    FROM users
    WHERE foo = 'bar'
";

I don't think there's anything wrong with controlling highlighing in comments. Perhaps a docblock tag would be better though. Maybe get in touch with the phpDocumentor creators and ask for some namespace in their keywords? https://phpdoc.org/contact

Something like this for example:

/**
  * @lang SQL
  */
 $foo = "
     SELECT *
     FROM users
     WHERE foo = 'bar'
 ";
JimmyBastos commented 5 years ago

I'm facing the same problem. Any update on this?

adjenks commented 5 years ago

If we used /**@DocBlocks*/, or something similar, you could easily toggle sections of code to use different languages embedded in strings and have no issue about identifying them. Much like how we insert a language into markdown with three back-ticks and then the language.

```SQL SELECT 'like so' FROM information_schema.tables WHERE true = true; ```

results in:

SELECT 'like so' FROM information_schema.tables WHERE true = true;
mbomb007 commented 4 years ago

I don't think a comment is enough. I think it should work with multiline regex.

KapitanOczywisty commented 4 years ago

@mbomb007 grammar engine is limited, but it's still possible to use heredoc:

$query = <<<SQL
    SELECT `column`
    FROM `table`
SQL;
keevan commented 2 years ago

@mbomb007 grammar engine is limited, but it's still possible to use heredoc:

$query = <<<SQL
    SELECT `column`
    FROM `table`
SQL;

Whilst it's possible to use heredoc, it might conflict with some existing coding standards on projects it's an alternative that might not work for everyone (highlighted here: https://github.com/atom/language-php/issues/385#issuecomment-1030601269)

Is the grammar engine issue fixed by this PR? https://github.com/atom/language-php/pull/303 . If my understanding is correct, it's currently using TextMate's grammar engine which does not support the level of multi-line matching we need for this to work, and the conversion to using tree-sitters (https://flight-manual.atom.io/hacking-atom/sections/creating-a-grammar/) is the way forward, should be more flexible, and will allow this issue to be fixed without weird workarounds + annotations?

blaumeise20 commented 2 years ago

Please update this! It's really annoying having to write the whole query in one line just to get syntax highlighting.