fix: BaseConnection::_escapeString() should accept Stringable
#8739
Conversation
This file contains 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
kenjis
added
the
bug
datamweb
approved these changes
Apr 9, 2024
michalsn
approved these changes
Apr 9, 2024
|
@kenjis shield support 7.4+ but Stringable is php>8 , how about? |
|
@datamweb No problem. If a user uses PHP 7.4, composer installs CI 4.4. |
kenjis
added
breaking change
kenjis
force-pushed
the
fix-BaseConnection-_escapeString
branch
from
ad23ec6
to
db17a77
Compare
kenjis
force-pushed
the
fix-BaseConnection-_escapeString
branch
from
db17a77
to
4825f89
Compare
kenjis
removed
the
docs needed
|
Added docs. |
samsonasik
reviewed
Apr 10, 2024
| @@ -20,6 +20,10 @@ Mandatory File Changes | |||
| Breaking Changes | |||
| **************** | |||
|
|
|||
| - **QueryBuilder:** Due to a bug fix, the parameter type of ``BaseBuilder::_escapeString()`` | |||
| has been changed from ``string`` to ``string|Stringable``. If you are extending | |||
| this method, update the method paramtere type. | |||
datamweb
approved these changes
Apr 10, 2024
| @@ -182,7 +183,7 @@ protected function _close() | |||
| /** | |||
| * Platform-dependant string escape | |||
| */ | |||
| protected function _escapeString(string $str): string | |||
| protected function _escapeString(string|Stringable $str): string | |||
| { | |||
| return str_replace("'", "''", remove_invisible_characters($str, false)); | |||
There was a problem hiding this comment.
Does remove_invisible_characters also allow Stringable?
| @@ -253,7 +254,7 @@ public function escape($str) | |||
| /** | |||
| * Platform-dependant string escape | |||
| */ | |||
| protected function _escapeString(string $str): string | |||
| protected function _escapeString(string|Stringable $str): string | |||
There was a problem hiding this comment.
Same question for pg_escape_string.
I think we need to add a string cast to both function calls.
There was a problem hiding this comment.
Thanks for the good insight!
kenjis
changed the title
BaseBuilder::_escapeString() should accept StringableBaseConnection::_escapeString() should accept Stringable
5 tasks
|
Go to #8756 |
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.
Description
It was accepted in 4.4 because of no strict types.
In 4.5, Stringable should be also accepted.
See also codeigniter4/shield#1092
Checklist: