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

Option to force uppercase SQL keywords #73

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,19 @@ use Doctrine\SqlFormatter\SqlFormatter;
echo (new SqlFormatter(new NullHighlighter()))->format($query);
```

#### Forcing uppercase for SQL keywords

If you wish to also force SQL keywords to be uppercase, you can do the following:

```php
<?php

use Doctrine\SqlFormatter\NullHighlighter;
use Doctrine\SqlFormatter\SqlFormatter;

echo (new SqlFormatter(new NullHighlighter()))->format($query, ' ', true);
```

Output:

![](http://jdorn.github.com/sql-formatter/format.png)
Expand Down
22 changes: 19 additions & 3 deletions src/SqlFormatter.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* SQL Formatter is a collection of utilities for debugging SQL queries.
* It includes methods for formatting, syntax highlighting, removing comments, etc.
*
* @link http://github.com/jdorn/sql-formatter
* @see http://github.com/jdorn/sql-formatter
*/

namespace Doctrine\SqlFormatter;
Expand All @@ -16,12 +16,14 @@
use function array_unshift;
use function assert;
use function current;
use function in_array;
use function preg_replace;
use function reset;
use function rtrim;
use function str_repeat;
use function str_replace;
use function strlen;
use function strtoupper;
use function trim;

use const PHP_SAPI;
Expand All @@ -47,7 +49,7 @@ public function __construct(?Highlighter $highlighter = null)
*
* @return string The SQL string with HTML styles and formatting wrapped in a <pre> tag
*/
public function format(string $string, string $indentString = ' '): string
public function format(string $string, string $indentString = ' ', bool $forceUppercase = false): string
Copy link
Member

Choose a reason for hiding this comment

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

How likely is it that somebody will want to sometimes uppercase SQL words, sometimes not in the same application? I wonder if this wouldn't be better as a constructor parameter. What do you think @goetas @rodnaph @othyn ?

Copy link

Choose a reason for hiding this comment

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

For me, unlikely - the purpose of the tool is standardisation.

(If someone did, they could create multiple instances perhaps).

Copy link
Member

@greg0ire greg0ire Aug 19, 2021

Choose a reason for hiding this comment

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

Too bad I did not use a more generic name when introducing Highlighter in #21 , this looks like it could be implemented more cleanly with a custom highlighter.

Copy link
Author

@othyn othyn Aug 19, 2021

Choose a reason for hiding this comment

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

Man it's so long ago I can't even remember my own use case! Haha 😅 I think... it was regarding an export tool I was writing to generate formatted SQL statements from Laravel migrations, in which having uppercased keywords suited that functionality better.

As for implementation, if you think it works better as a constructor option then I'm happy to oblige.

Copy link
Member

Choose a reason for hiding this comment

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

That or maybe there should be an abstraction next to Highlighter. Highlighting is about adding things before and after, there could be another interface in charge of changing the case, so that you would have

            $highlighted = $this->highlighter->highlightToken(
                $token->type(),
                $this->changeCase($token->value())
            );

and echo (new SqlFormatter(new NullHighlighter(), new UppercaseSomethingSomething()))->format($query)

Naming ideas welcome, and let's wait for @goetas to look at this, he's good at design.

Copy link
Member

Choose a reason for hiding this comment

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

@SenseException can you please rephrase? I don't understand 😅

Copy link
Member

Choose a reason for hiding this comment

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

$forceUppercase and also $indentString are used for the representation of the SQL string output and I see both as options that could be injected as a config for the second constructor argument. While $indentString has to stay in the format function for version 1, I can imagine that a future 2.0 can contain this info for "use uppercase" and "use spaces as indent" inside e.g. a Config object that SqlFormatter (or somewhere else) gets injected.

Copy link
Member

@greg0ire greg0ire Aug 20, 2021

Choose a reason for hiding this comment

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

Maybe we should have a fromConfig named constructor that should call a somewhat complicated constructor in v2 yeah. PHP 8 usage could be SqlFormatter::fromConfig(indentString: '>', case_strategy: UPPERCASE_SQL_KEYWORDS, highlighter: HTML) and then it would result in the constructor being called with the right implementations, that way we don't have to maintain a config object with properties, getters and setters, it all gets validated and thrown away in the named constructor, and people can still swap in / swap out their own implementations by using the normal constructor.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of a named constructor. Not having a Config object to maintain has its advantages, but getter and setter methods aren't needed considering typed properties that are already part in 7.4.

Choose a reason for hiding this comment

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

I sustain it, but adding a $forceUppercase option would add only ONE option.

Please read my last suggestion instead : #85

{
// This variable will be populated with formatted html
$return = '';
Expand All @@ -71,9 +73,23 @@ public function format(string $string, string $indentString = ' '): string

// Format token by token
while ($token = $cursor->next(Token::TOKEN_TYPE_WHITESPACE)) {
// Uppercase reserved words
$uppercaseTypes = [
Token::TOKEN_TYPE_RESERVED,
Token::TOKEN_TYPE_RESERVED_NEWLINE,
Token::TOKEN_TYPE_RESERVED_TOPLEVEL,
];

// Uppercase transformation if desired
if ($forceUppercase && in_array($token->type(), $uppercaseTypes)) {
$tokenValue = strtoupper($token->value());
} else {
$tokenValue = $token->value();
}

$highlighted = $this->highlighter->highlightToken(
$token->type(),
$token->value()
$tokenValue
);

// If we are increasing the special indent level now
Expand Down
10 changes: 10 additions & 0 deletions tests/SqlFormatterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ public function testFormat(string $sql, string $html): void
$this->assertEquals(trim($html), trim($formatter->format($sql)));
}

public function testFormatUpperCase(): void
{
$formatter = new SqlFormatter(new NullHighlighter());

$actual = $formatter->format('select a from b where c = d;', ' ', true);
$expected = "SELECT\n a\nFROM\n b\nWHERE\n c = d;";

$this->assertEquals(trim($expected), trim($actual));
}

/**
* @dataProvider highlightData
*/
Expand Down