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

Custom DQL functions: TypedExpression returning StringType does not cast to string #11537

Open
janedbal opened this issue Jul 1, 2024 · 5 comments

Comments

@janedbal
Copy link
Contributor

janedbal commented Jul 1, 2024

While implementing better expression type inference in phpstan-doctrine, I added support even for TypedExpression as those are the only way how to have type inference with custom functions.

But the problem is that it is designed to use Type::convertToPHPValue which is no-op for StringType (it keeps whatever the value is, no casting performed). This means that all TypedExpressions returning StringType are actually not typed at all.

I kept this exception in phpstan-doctrine, but this feels like a design flaw and can definitelly cause some WTF moments.

Obviously, anybody can solve it by custom Type that do cast to string, but that requires deeper knowledge of how things work.

use Doctrine\DBAL\Types\Type;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Query\AST\Functions\FunctionNode;
use Doctrine\ORM\Query\Parser;
use Doctrine\ORM\Query\SqlWalker;
use Doctrine\ORM\Query\TokenType;

class Date extends FunctionNode implements TypedExpression
{
    public $date;

    public function getSql(SqlWalker $sqlWalker): string
    {
        return 'DATE(' . $sqlWalker->walkArithmeticPrimary($this->date) . ')';
    }

    public function parse(Parser $parser): void
    {
        $parser->match(TokenType::T_IDENTIFIER);
        $parser->match(TokenType::T_OPEN_PARENTHESIS);

        $this->date = $parser->ArithmeticPrimary();

        $parser->match(TokenType::T_CLOSE_PARENTHESIS);
    }

    public function getReturnType(): Type
    {
        return Type::getType(Types::STRING); // "broken"
    }
}

I dont really have any proposal of how to make this better. Maybe just document it?

@janedbal
Copy link
Contributor Author

janedbal commented Jul 1, 2024

Related issues:

@derrabus
Copy link
Member

derrabus commented Jul 1, 2024

StringType is meant to be used on VARCHAR data coming from a database server. Since no DBMS supported by DBAL would return a VARCHAR as something other than a string, we don't perform any type conversion here.

@janedbal
Copy link
Contributor Author

janedbal commented Jul 1, 2024

I understand that, but since you are using Types even in other contexts (like TypedExpression), it causes issues like described above.

@stof
Copy link
Member

stof commented Jul 1, 2024

given that the ORM currently does not allow registering custom boolean functions for instance (see the list of Configuration::addCustom*Function methods), adding an explicit cast in StringType could actually break some existing cases.

janedbal added a commit to janedbal/orm that referenced this issue Jul 11, 2024
@janedbal
Copy link
Contributor Author

adding an explicit cast in StringType could actually break some existing cases

I understand that.

given that the ORM currently does not allow registering custom boolean functions for instance

I dont understand how that is related. I can easily register e.g. custom NOT function.


I actually dont understand what is the addCustom*Function actually distinguishing. All those methods in Doctrine\ORM\Query\Parser do the same thing:

  • CustomFunctionsReturningStrings
  • CustomFunctionsReturningNumerics
  • CustomFunctionsReturningDatetime

Feels like it actually does nothing.

janedbal added a commit to janedbal/orm that referenced this issue Aug 6, 2024
Partially related to doctrine#11537

Co-authored-by: Claudio Zizza <859964+SenseException@users.noreply.github.com>
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

3 participants