SQLite: Escape numbers as keywords #4247
Conversation
| */ | ||
| public function isKeyword($word) | ||
| { | ||
| return parent::isKeyword($word) || is_numeric($word); |
There was a problem hiding this comment.
Is this an alternative to #4246? 5 doesn't look like an SQLite keyword, so this change doesn't look right. As pointed out earlier, the problem is that an invalid key name is generated in the first place, not that it's not escaped.
There was a problem hiding this comment.
Yes and no, no one prevents a user to use numbers as FK names so it can break for them in the future. The reason its here is to not clutter \Doctrine\DBAL\Schema\AbstractAsset::getQuotedName with some numerical checks, esp. as i don't know how other databases handle this and at the end this was the fastest (aka least code change/restructuring) change
So to compare them both: The first and the second PR fix the same problem by fixing two independent bugs which in combination make changing tables with FKs impossible.
There was a problem hiding this comment.
We're currently reevaluating the approach to quoting names leaning towards the user being responsible for quoting since the existing magic is unreliable anyways.
no one prevents a user to use numbers as FK names so it can break for them in the future
I believe it never worked, so it's a good enough preventing measure.
Regardless of that, it doesn't look like a bugfix so 2.10.x is not the right place for this change.
There was a problem hiding this comment.
At least it would be the smallest hotfix for 2.10.x as its currently broken for anyone changing tables so i would consider it a bugfix? But i totally agree with the point that it definitely does not feel right to add it there (but again, its the "minimum change"/the change that i am currently comfortable to do in this project 😁)
Summary
This fixes #4243 by escaping numbers