Use literal-string type for QueryBuilder::where()

[craigfrancis]

Hi Ondřej, this is following up on an email on the 26th August, where you suggested updating the PHPStan stub files here.

I have tried to get Doctrine ORM updated, but no luck so far (see also Issue 8933).

I've been running some tests, and cannot find any problems so far, but I'd still like to be cautious and limit it to a single method. I'll keep an eye out for any feedback, and try to help resolve any issues.

[craigfrancis]

If it’s ok with you, I’ll try to improve the use of array<mixed> in a separate PR, because that’s going to involve a bit more digging to make sure I’ve got it right (or maybe someone with a bit more experience could make a suggestion?)

Ref QueryBuilder::where() which is passed via $dqlPart in querybuilder::add().

[ondrejmirtes]

Hi, it seems a bit weird to introduce the type in a single place like that. I'd rather introduce it in all places where it makes sense and put it behind a bleedingEdge toggle, that's going to be more useful.

[ondrejmirtes]

And what about starting with DBAL first and ORM later?

[craigfrancis]

I'm happy to do that, but before I do, can I explain why I took this approach (your thoughts are very welcome)...

---

I've been trying to work out the best way to introduce developers to this concept. My concern with introducing it to all methods, even if it's initially behind a toggle like bleedingEdge, is that developers are likely to switch it off if they get too many errors at once (while they should update their baseline, I find some developers use ignoreErrors).

And while these errors have a very high chance of being vulnerabilities (i.e. should be fixed); I suspect teaching about literal-string, via a single method, and only when using a simple string (rather than the more complex array/object inputs), would mean that developers will only have, at most, a few quick and easy changes to make.

This is the approach taken with Propel ORM and RedBean.

I'm trying to use a bit of psychology - an error is kinda telling someone they are wrong, one of the more common human reactions is to get defensive. But, if it's an easy change, they are more likely to make that change and move on. Over the next few hours/days, they continue to think about it, even if subconsciously, because they spent time/energy on it. And because they have made a small investment of time/energy into this, they will hopefully tweak their approach, and may even think about using it in other places as well.

As to why this specific method, it's the one I see a reasonable amount of problems with (anecdotally). Fortunately most of the ORM APIs are pretty safe; it's the QueryBuilder that's the main problem, and while I could have updated methods like ->leftJoin() I doubt many people would have noticed that. In contrast, DQL is a nightmare, so many developers incorrectly see it as safe SQL; and DBAL (e.g. $conn->prepare()) can get complicated, which is why I think we need to get developers used to literal-string via a simple and fairly well used method first.

A concern raised by one of the Doctrine maintainers is that it might cause some false positives. I'm not sure what they would be (and no examples have been given), but I cannot prove there won't be any, especially when thousands of developers are using Doctrine. So I'd like to go with the cautious approach, which we can revert and re-think about if there are complaints.

That said, you know the developers who use PHPStan far better than I do; and, if you prefer, I'd be happy to create a PR to "introduce it in all places where it makes sense".

---

As an aside, I am wondering about extending the comment in the stub file, so anyone looking at why this error happened, has a good chance of learning how to fix it. But I'm not sure this is the right place to do this.

/**
 * @param literal-string|object|array<mixed> $predicates
 * @return $this
 *
 * To include user values, you should use `->setParameter()`, e.g.
 *
 * $qb
 *   ->select('u')
 *   ->from('User', 'u')
 *   ->where('field_name = :user_value')
 *   ->setParameter('user_value', $user_value);
 *
 * If the user needs to specify the field, use an 'allow-list' approach,
 * to ensure they can only specify fields that you allow, e.g.
 *
 *   $fields = ['name', 'address', 'email'];
 *
 *   $field_id = array_search($unsafe_user_field, $fields);
 *
 *   $field = $fields[$field_id];
 *
 * Or if you want to use aliases for those fields:
 *
 *   $fields = [
 *     'name' => 'u.full_name',
 *     'email' => 'u.email_address',
 *     'address' => 'u.postal_address',
 *   ];
 *
 *   $field = ($fields[$unsafe_user_field] ?? 'u.full_name');
 *
 * These return a `literal-string` that can be used like this:
 *
 *   $qb
 *     ->select('u')
 *     ->from('User', 'u')
 *     ->where($field . ' = "my_value"')
 *
 */

[staabm]

Fwiw, I am planning to add a „strict“-mode to phpstan-dba, which would enforce literal strings on query-apis

[ondrejmirtes]

It's interesting how different our field of views are :) You're trying to carefully and slowly educate users about this concept. I want to make sure it's applied consistently and make sure the technical implementation is correct and to gather feedback about the feature.

Over the next few hours/days, they continue to think about it, even if subconsciously, because they spent time/energy on it.

We have very little control over that here. I think it'd be puzzling for users why this wasn't done in one step but gradually over many versions, it might make them more annoyed.

More people will learn about this if it's used in more places. I think the more data points we get the better so I'd like to use it everywhere, with the bleedingEdge toggle (and enable it for everyone in 2.0). That might be bit of a challenge in an extension. But you can implement this interface https://github.com/phpstan/phpstan-src/blob/master/src/PhpDoc/StubFilesExtension.php, and register it via that tag in extension.neon. Make sure to ask for %featureToggles.bleedingEdge% in the constructor :) This is how: https://phpstan.org/developing-extensions/dependency-injection-configuration#autowiring

[craigfrancis]

That's fair, and you're probably right about annoying people if it's done over many versions... I was roughly thinking of 4 steps, firstly this one (just to get some feedback), second all QueryBuilder methods, third any DQL inputs (the one I'm most worried about), and finally anything else.

I'll put together a proposal over the next few weeks (i.e. when normal work isn't getting in the way)... and I'll see if @staabm can help, as my experience with Doctrine is a bit limited (it took quite a bit of digging just to be sure of this change, and that didn't include the object/array inputs).

If you have any other thoughts, please let me know; and thanks again for everything you've done :-)

[staabm]

I have nearly zero experience with doctrine.

[craigfrancis]

@ondrejmirtes, can I just check I'm going in the right direction with: craigfrancis@e65bbc4

I've created a new /src/LiteralString/ folder, containing the new stub files (will eventually use literal-string for all relevant method arguments), and these files will be returned by LiteralStringStubFilesExtension.php for anyone using bleedingEdge.

[ondrejmirtes]

@craigfrancis Mostly yeah, but you need to have different stubs for QueryBuilder for bleedingEdge/nonBleedingEdge. E.g. remove the path from extension.neon and have a different path returned in if ($this->bleedingEdge) { ... } else { ... }...

[craigfrancis]

@ondrejmirtes Good point, it wouldn't combine them.

I've made that change, but did so by tweaking the root $path: master...craigfrancis:literal-string

Should I create a new pull request now, so anyone using bleedingEdge can start to give feedback, or shall I come back to you once I've worked out all of the other parameters that need to be updated?

[ondrejmirtes]

Can we perhaps have '/stubs/ORM/QueryBuilder.stub' and '/stubs/bleedingEdge/ORM/QueryBuilder.stub'? Otherwise yeah, update the PR :) We can do it in multiple steps if it's under bleedingEdge.