Start experiment with 'literal-string' type#1788
Merged
dereuromark merged 4 commits intopropelorm:masterfrom Oct 11, 2021
Merged
Start experiment with 'literal-string' type#1788dereuromark merged 4 commits intopropelorm:masterfrom
dereuromark merged 4 commits intopropelorm:masterfrom
Conversation
dereuromark
reviewed
Oct 11, 2021
dereuromark
approved these changes
Oct 11, 2021
Contributor
Author
|
Yep, sorry, being lazy and didn't re-run |
Contributor
Author
|
Thanks @dereuromark ... I'll try to keep an eye out for any issues that are reported, and if everything's ok in a few months time, I'll make a new PR that covers a few more methods. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both Psalm 4.8 and PHPStan 0.12.97 have introduced the
literal-stringtype.This checks that a string has been created by the developer (defined in the source code), so mistakes that lead to Injection Vulnerabilities can be identified, e.g.
By setting the
$clauseparameter toliteral-string|array, while thearrayversion won't be checked, at least the string can be (ensuring it does not contain unsafe user data).And
literal-stringis quite forgiving, in that it works with variables, and allows string concatenation (assuming both areliteral-stringvalues), so things like conditional where clauses are fine, e.g.That said, I still want to be cautious (I don't want to create issues for the users of Propel). Which is why I'd like to start with
->where(), see what feedback we get (I'm happy to help with that), and if we get a positive response, we can update other methods. The last one will probably be$statementin$con->prepare(), as I appreciate that its input is a bit more complicated.@dereuromark, we briefly talked about this concept in April, in regards to the is_literal RFC. While it didn't pass (I'll try again next year, once I've addressed the concerns people had), Psalm/PHPStan have now added support, so we can introduce this check for developers that use Psalm (level 3 or stricter) or PHPStan (level 7 or stricter)... where it will help them identify mistakes often created by junior developers making a "quick edit".