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

Incorrect type in SelectStatement::$expr when query has CASE-clause in selected fields #552

Open
Daniel-I-Am opened this issue Feb 29, 2024 · 5 comments

Comments

@Daniel-I-Am
Copy link

The public field SelectStatement::$expr is defined in a PHP docblock as an Expression[]. When parsing a query with a CASE statement, a CaseExpression object is present in this list, despite CaseExpression not extending Expression. This causes issues when explicitly requiring the Expression type as should be returned by SelectStatement::$expr.

Reproduction case

<?php

declare(strict_types=1);

use PhpMyAdmin\SqlParser\Components\Expression;
use PhpMyAdmin\SqlParser\Parser;
use PhpMyAdmin\SqlParser\Statements\SelectStatement;

include_once 'vendor/autoload.php';

$p = new Parser('SELECT a, CASE WHEN b IS NOT NULL THEN 1 ELSE 0 END as c FROM t');

$stmt = $p->statements[0];
if (! $stmt instanceof SelectStatement) {
    throw new Exception('Could not parse select statement');
}

function acceptExpression(Expression $e): void {}

$expressions = $stmt->expr;
foreach ($expressions as $expression) {
    acceptExpression($expression);
}

Expected behavior

I would expect this script to not error out as the $stmt->expr returns Expression[] according to its PHP docblock. Each element should therefore be compatible with Expression in my acceptExpression function.

Real behavior

$ php test.php
PHP Fatal error:  Uncaught TypeError: acceptExpression(): Argument #1 ($e) must be of type PhpMyAdmin\SqlParser\Components\Expression, PhpMyAdmin\SqlParser\Components\CaseExpression given, called in /path/to/test.php on line 22 and defined in /path/to/test.php:18
Stack trace:
#0 /path/to/test.php(22): acceptExpression()
#1 {main}
  thrown in /path/to/test.php on line 18
@williamdes
Copy link
Member

Hello
What version of sql-parser are you using?

@Daniel-I-Am
Copy link
Author

This is on the latest release of sql-parser, 5.9.0, with PHP 8.2.

@williamdes
Copy link
Member

@MauricioFauth could you please have a look?

@kamil-tekiela
Copy link
Contributor

kamil-tekiela commented Feb 29, 2024

This was added #88

I noticed this issue before but I have no idea how to fix this. CaseExpression cannot inherit from Expression, but all of the code is written around the value being only Expression.

@niconoe-
Copy link
Contributor

#583 Defines now that SelectStatement::$expr can be of type CaseExpression[].
@Daniel-I-Am , your snippet will still cause an error as this is just a change in the PHPDoc, but at least, the PHPDoc will be valid and you could change your behavior to

function acceptExpression(CaseExpression|Expression $e): void {}

As @kamil-tekiela said, CaseExpression cannot inherit from Expression, so I think that's the best we could provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants