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 support for Oracle LOOP statement #116

Merged
merged 1 commit into from
May 23, 2024

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek marked this pull request as ready for review May 22, 2024 15:43
@greg0ire
Copy link
Member

Fix fatal error for Oracle LOOP statement

What fatal error are you talking about?

@mvorisek
Copy link
Contributor Author

Doctrine\SqlFormatter\Tests\SqlFormatterTest::testFormat with data set #51 ('BEGIN\n  FOR i IN 1..5\n  LOO...\nEND;', 'BEGIN\n  FOR i IN 1..5 LOOP\n...\nEND;')
ValueError: str_repeat(): Argument #2 ($times) must be greater than or equal to 0

...\src\SqlFormatter.php:87
...\src\SqlFormatter.php:292
...\tests\SqlFormatterTest.php:47

(when the tests are run /wo the fix)

@mvorisek mvorisek changed the title Fix fatal error for Oracle LOOP statement Fix indent for Oracle LOOP statement May 22, 2024
@greg0ire
Copy link
Member

Apparently, situations where the indent level ends up being negative are supposed to be handled by highlighting the last token in red:

if ($indentLevel < 0) {
// This is an error
$indentLevel = 0;
$return .= $this->highlighter->highlightError($token->value());
continue;
}

Should we address that first?

@mvorisek
Copy link
Contributor Author

mvorisek commented May 22, 2024

Apparently, situations where the indent level ends up being negative are supposed to be handled by ...

Should we address that first?

Any warning added to a query is unwanted - it corrupts the query, so yes, this should be addressed, but for 1.4.x by parsing the tokens into matched indentation blocks. If the tokens are unable to be matched (like something is opened but not closed and vice versa), this should be in no indentation, but without any error/warning added to the formatted query.

This PR fixes known LOOP not opening an indentation block but END closing it. Even with the fix discussed above, we need this PR to format such SQL properly. So it is safe to land this PR now as needed anyway.

@greg0ire
Copy link
Member

OK so:

  • there is a bug, and you are not fixing it here
  • support for LOOP is missing, and that is what you are contributing here (so it's a new feature, and you are targeting 1.5.x), and it happens to make a crash disappear, but the same crash can happen in many other situations.

Please rework your commit message to make it more apparent that this is not a fix.

@greg0ire greg0ire added the enhancement New feature or request label May 23, 2024
@greg0ire greg0ire changed the title Fix indent for Oracle LOOP statement Add support for Oracle LOOP statement May 23, 2024
@mvorisek
Copy link
Contributor Author

For the general issue you mentioned I created #118 👍

@mvorisek mvorisek requested a review from greg0ire May 23, 2024 08:19
@greg0ire greg0ire merged commit 4d6bc8a into doctrine:1.5.x May 23, 2024
10 checks passed
@mvorisek mvorisek deleted the fix_oracle_throw branch May 23, 2024 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants