Fix some sql highlighting

[keevan]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions

Description of the Change

Combined screenshot of result

Before:

After (note $editors not being highlighted when SQL syntax is applied is a separate issue):

3 main changes to allow for SQL syntax highlighting in strings, in new scenarios.

1. Allows lowercased keywords (e.g. "select * from users")

2. Query starting on next line of string opener (requires --sql on opening line).

3. Query starting with a parenthesis will be allowed in the SQL begin check. (e.g. "(select 1) union all (select 2)"

Alternate Designs

1. An alternative to this might be to repeat the same list of keywords in lowercase, so it prevents false positives such as the string "Select from the following:" from being incorrectly highlighted. This doesn't prevent the problem entirely though, and I'm probably still okay making a non-important string have weird colors than to not have this option for people who prefer code to - and I quote - "be screaming at them".
2. Alternative options were discussed in this thread: Embedded SQL-Syntax only works for inline SQL #87
   Until tree-sitters become a reality, I think having a 'experimental / opt-in' option might make more people happy in the mean time. At least, I would be. An option is better than none / waiting imo. I wouldn't necessarily enforce it throughout the codebase though.
3. Heredocs were proposed but PHPCS might disallow such usage in the first place, and therefore could not be an option.

Benefits

More syntax highlighting options for SQL in PHP strings, particularly ones that probably should have worked out of the box (1,2)

Possible Drawbacks

False positives (1 proposed) - for the string examples mentioned above.

Otherwise, if it's as the (alternative design 1), this would mean that sPonGebob notation will not be supported. (See https://sqlfum.pt/)

Applicable Issues

#87
#449 (mentions but not its own issue)

[keevan]

• Should fix up --sql matching to be at least -- sql instead, because --sql is not a valid comment and syntax highlighting should not incorrectly kick off earlier than intended.

[KapitanOczywisty]

1. Allows lowercased keywords (e.g. "select * from users")
3. Query starting with a parenthesis will be allowed in the SQL begin check. (e.g. "(select 1) union all (select 2)"

These are ok for me

2. Query starting on next line of string opener (requires --sql on opening line).

However I don't see any point for that one. There Is Heredocs for such cases, and -- sql seems to not be used at all in that context in the wild: https://grep.app/search?q=%5B%27%22%5D--%5Cs%2Bsql&regexp=true&filter[lang][0]=PHP

Also please add some tests for new features.

[KapitanOczywisty]

1. An alternative to this might be to repeat the same list of keywords in lowercase, so it prevents false positives such as the string "Select from the following:" from being incorrectly highlighted.

This is valid concern, but Select is also used for sql (examples). I'm not sure if it's worth adding other variants than uppercased, since it can break a lot of code. But this is something to be discussed.

Heredocs were proposed but PHPCS might disallow such usage in the first place, and therefore could not be an option.

Rule was created when heredocs weren't able to handle indentations, this is not the case now. By grep.app Squiz.PHP.Heredoc.NotAllowed was disabled more often than -- sql was ever used.

[keevan]

Rule was created when heredocs weren't able to handle indentations, this is not the case now. By grep.app Squiz.PHP.Heredoc.NotAllowed was disabled more often than -- sql was ever used.

Fair point. It seems the indent-able heredocs was introduced in php 7.3. Also yeah I wouldn't be suprised if -- sql was ever used anywhere, since it started from a need here: #87 (comment)

I still personally prefer the option to trigger highlighting with -- sql because the effort to add it to existing query blocks is simpler than converting all the bits to heredocs. But that's what forks are for 😄 (Though I wish there were an option to toggle this on / off in settings)

Also thanks for the feedback. I'll try to add some tests for the new stuff when I get the chance.

[keevan]

@KapitanOczywisty I've applied your suggestions and added some tests based on tests that were already there. Let me know if you prefer I split it out to a separate it block, though I feel like it made more sense where I've added it.

Ran specs locally - seems okay. Back over to you.

[adjenks]

Something that really bugs me is that the CTE keyword WITH doesn't trigger syntax highlighting. Would that be fixed if I added the --sql?

[keevan]

Something that really bugs me is that the CTE keyword WITH doesn't trigger syntax highlighting. Would that be fixed if I added the --sql?

Ah, the --sql keyword was something I thought about adding, but has been removed as @KapitanOczywisty did not want to introduce new custom syntax to trigger the syntax highlighting.

I tried locally, and it seems like it is highlighting as expected:

The only time it doesn't is if the string starts with a ;. @adjenks, Do you have an example of when you'd expect it to start highlighting? Perhaps it might be simple enough to include as part of this PR as well to hopefully get it over the line.

[adjenks]

@keevan It doesn't exist in phpDocumentor, but I always thought that putting the language in a doc-block made the most sense. It's common format for documenting code.

Something like...
/** @lang sql */
$query = 'SELECT 1';

There's no lang tag, but, here is the var tag as an example:
https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/var.html#examples

I don't think it should be inside the string itself because then you add some overhead to the query being sent to the server, albeit not much.

Good to see that WITH is mostly working though, last time I checked it wasn't. Guess I haven't tested it for a while.

[KapitanOczywisty]

The only time it doesn't is if the string starts with a ;.

What is the purpose of ; before the query? I don't think it is functional unless you are creating multi-query - which is not the best idea in php.

It doesn't exist in phpDocumentor, but I always thought that putting the language in a doc-block made the most sense. It's common format for documenting code.

It won't work for TextMate (or even Tree-sitter) syntax highlighting, it would be way too much added complexity, if even possible.

I don't think it should be inside the string itself because then you add some overhead to the query being sent to the server, albeit not much.

This is over-optimization, striping comments takes nanoseconds. Updating table indexes can have real impact on performance, not removing comments. Measure, optimize and measure again.

[keevan]

What is the purpose of ; before the query?

I don't use it myself, I only observed examples online when searching for examples for @adjenks where highlighting did not apply.

With that said, if there's nothing left that needs to be changed, I'd like these changes to be reviewed and merged in if possible 😃

[KapitanOczywisty]

With that said, if there's nothing left that needs to be changed, I'd like these changes to be reviewed and merged in if possible 😃

This will take a while, also with possible false positives from case insensitive keywords, I'd like to see the input from the community.

[adjenks]

I'd like to see the input from the community.

I still think that all syntax highlighters should offer a way of indicating the language of a string by writing it in a preceding comment, much like how you often can in markdown by writing the language after three back-ticks.

Markdown:

```js
console.log('hello');
```
Result:

console.log('hello');

Perhaps we could have a vote on anything that you want input on?

We could vote on manual language selections here using reactions:

(React 😄) Option 1 - Inside the text itself:

$query = '--sql
SELECT 1';

(React 🎉) Option 2 - Inside a preceding docblock:

/** @lang sql */
$query = 'SELECT 1';

(React 🚀) Option 3 - Inside a preceding non-docblock comment:

//L:sql
$query = 'SELECT 1';

(React 😕) Option 4 - None, only automatic language detection:

$query = 'SELECT 1';

I think auto-detection is fine, but if you can just manually select I think it would solve some problems and improve documentation and processing in some cases.

[KapitanOczywisty]

@adjenks First of all atom is dead, so there is no point discussing this PR any further. If you're using vscode there will be fork by me or somebody else and there you can continue - with vscode in mind.

Second thing options 2 and 3 are not viable to implement, textmate grammar doesn't have "memory" to keep track of language labels and use them later. Option 1 is non-standard and not popular (invented in this very PR), this is not something which should be added for hundreds of thousands of users.

And finally there is heredoc <<<SQL which is used, popular and since php 7.3 properly formattable.

There was somwhere issue to improve semantic tokens in vscode which would allow to support stuff like /** @lang sql */ without major overhead, however I don't think it is on the radar right now.

[adjenks]

@KapitanOczywisty Ah okay. Thank you for all the info.

I was interested in highlighting for both applications since I was using both when I joined the discussion. Someone had given me the impression that VSCode shared this library. Feel free to point me to the correct library for VSCode if one has been made.

That's unfortunate about the unviability. The semantic token issue you mentioned would be nice. I'll see if I can find it.

[KapitanOczywisty]

Someone had given me the impression that VSCode shared this library.

It is still using it, but nobody is merging anything and repository will be archived in mid december.

Feel free to point me to the correct library for VSCode if one has been made.

Not yet, but we're thinking about how to proceed. I'll leave note about the new repository in the issues when time comes.