class-string vs literal-string in QueryBuilder

[mhujer]

With the recent addition of literal-types to QueryBuilder (https://github.com/phpstan/phpstan-doctrine/pull/327), I'm getting warnings when passing class-string there: Parameter #1 $from of method Doctrine\ORM\QueryBuilder::from() expects literal-string, class-string given

This is the simplified example to reproduce the issue:

    /**
     * @param class-string $className
     */
    public function getSortable(
        string $className,
    ): ?Sortable
    {
        return $this
            ->entityManager
            ->createQueryBuilder()
            ->select('sortable')
            ->from($className, 'sortable')
            ->getQuery()
            ->getOneOrNullResult();
    }

What is do you think is the correct way to handle this? To me it seems that class-string is more specific type than the literal-string.

cc @VincentLanglet

[ondrejmirtes]

I guess that class-string should pretty much be safe. WDYT @craigfrancis?

[VincentLanglet]

We either have to use class-string&literal-string or PHPStan (and psalm) need to consider that a class-string is a literal string. The same problem exist currently for psalm: https://psalm.dev/r/a12d74da73.

And the same question could be asked for some other string type ; like numeric-string.

[ondrejmirtes]

And the same question could be asked for some other string type ; like numeric-string.

I don't think so, numeric-string can be dangerous and coming from the database. But it's also right that class-string can contain emojis etc. so it's probably not that safe...

[craigfrancis]

Good point, the PHP implementation from Joe Watkins treats \stdClass::class as a literal:

var_dump(is_literal(\stdClass::class)); // true

class example {
}
$e = new example();
var_dump(is_literal(example::class)); // true
var_dump(is_literal($e::class)); // true

$class = new \ReflectionClass('example');
var_dump(is_literal($class->getName())); // true

If I'm reading the C patch correctly, I think that's from line 7.

I think that makes sense, as it's seen as a string, and it was written by the developer.

Is there a list of all the ways a class-string can be created? just so I can check them all... or is it one of those things where we can just assume it's going to be fine?

---

As to numeric-string, yeah, there was a long argument on the PHP Internals mailing list, look for the is_noble() suggestion from Scott Arciszewski... in short, while we cannot find a way that numbers can lead to an exploit, the idea was rejected (was seen as a messy idea, not as pure, with some people not feeling like they can trust it).

[VincentLanglet]

@orklah how is handled class-string vs literal string in psalm ?

I thought they were a literal-class-string https://github.com/vimeo/psalm/blob/dc1132a9155cf8879072ebad0bb7053674e6f0f7/src/Psalm/Internal/Type/TypeCombiner.php#L954 but it doesn't seem supported https://psalm.dev/r/2a024c59f9

And class-string is not considered as a literal-string https://psalm.dev/r/a12d74da73 (but might should)

[orklah]

There's a confusion due to bad naming in Psalm's code in retrospect.

TLiteralClassString is a TLiteralString whose value happens to be a known class-string and is denoted like stdClass::class: https://psalm.dev/r/b8413cbf35. (An example of a TLiteralString would be 'foo')

The literal-string type is a TNonspecificLiteralString. Literally a function expecting this would expect "any string whose value is a known string from the POV of the developper, even if SA can't know what is this string (hence the "non specific")"

So in your second example, the error is legit because "class-string" could very well be the value from $_GET['class_string'] after being checked in class_exists, so class-string may not be a literal-string. We don't currently have a TNonSpecificLiteralClassString who could denote any class-string whose value is hard coded by the developper

[VincentLanglet]

Indeed, by the definition of literal-string, in this exemple https://phpstan.org/r/2487713a-4290-4785-a89e-da1f274d754b the class-string is not a literal string @craigfrancis. But, I don't see how you could do some nasty injection with a class-string, so I would say it's also a safe string.

[mitelg]

hey guys :wave:

today I tried to enable bleeding-edge features and this problem occurs quite often then.

Is there a way to disable this specific feature and have all other bleeding-edge features still available? :thinking:

[ondrejmirtes]

@mitelg There isn't. This is the way forward. The point of bleedingEdge is that this is how it's going to be for all users in the next major version. It's better to improve the experience for everyone (I'm keen to hear any specific problems or situations you don't agree with so we can work on them) rather than being able to completely turn this off.

Of course there's https://phpstan.org/user-guide/ignoring-errors if you need it.

[mitelg]

okay :+1:

most of the issues are related to the OP, so class-string != literal-string. But if I read it here correctly, there is currently no solution to this?

[ondrejmirtes]

Maybe typehint class-string&literal-string.

Or maybe we don't actually need to require literal-string in the from method, I don't know if literal-string actually adds any value on QueryBuilder as opposed to EntityManager::createQuery() where you directly write DQL.

[mitelg]

Sounds reasonable :+1: But, I am not that deep into that, I just wanted to share my experience here :slightly_smiling_face:

For now, I will ignore this specific error and will look forward to a solution :grin:

[VincentLanglet]

Looking at the __toString method, I think the literal string is need

public function __toString()
    {
        return $this->from . ' ' . $this->alias .
                ($this->indexBy ? ' INDEX BY ' . $this->indexBy : '');
    }

So I would say that class-string&literal-string is the way to do it.

[ondrejmirtes]

I think this one is solved.

[mhujer]

@ondrejmirtes yes, I'm now using this:

    /**
     * @phpstan-param class-string&literal-string $className
     */
    public function getSortable(
        string $className,
    ): ?Sortable
    {
        return $this
            ->entityManager
            ->createQueryBuilder()
            ->select('sortable')
            ->from($className, 'sortable')
            ->getQuery()
            ->getOneOrNullResult();
    }

[github-actions[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.