Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Apply explicit string casts for strtolower() and urlencode() functions #282

Closed
wants to merge 1 commit into from

Conversation

sebsnake
Copy link

(cherry picked from commit ebb7c6c)

While we were migrating our application to PHP 8.1, some of our phpunit tests failed, as they somehow managed to get "null" inside symfony internals, where now strings are the only allowed argument types (resulting in deprecation warnings for now, but we configured our test suites to fail on those as well).

This happend for cases of strtolower() and urlencode() functions. Thus, we simply got our application (test suites) working again by applying explicit string casts to those function calls via simple search-and-replace statements throught all source files. I am aware that this results in possibly many non-required casts, but we wanted to be sure and thus applied the patch to all uses of these functions.

Thought this might be useful for you as well, thus I made this pull request.

@mentalstring
Copy link
Contributor

Seems a bit of a heavy solution which negates most of the advantages of having type checks: sometimes the nulls shouldn't be getting in there in the first place and the issue may be on the inputs: with the overall casting, we would miss the warnings/errors when on that.

I've been using the latest release on a medium usage website and only spotted one deprecation warning at:

$arguments["%$key%"] = htmlspecialchars($value, ENT_QUOTES, sfValidatorBase::getCharset());

where $value sometimes is null when using sfValidatorSchemaCompare but I haven't manage to isolate the issue into a trivial trigger, so haven't go around submitting a fix.

Anyway, that's the only place that I've found a deprecation warning so far. Personally I wouldn't apply a blanket casting.

@thirsch
Copy link
Collaborator

thirsch commented Feb 20, 2023

I wouldn't add it to every method as well. As discussed in #283, we are about to move to a higher platform minimum version (7.4 as of latest discussions) and therefore a future change might bring typehints to the method signatures, bringing type errors back to who is responsible for calling them correctly.

@connorhu
Copy link
Collaborator

I am using the latest version of symfony1 under 8.2 without any deprecation warnings. It is very strange that so many errors were found. If a method does not cause an error, it should not be changed.

@sebsnake
Copy link
Author

Your arguments sound convincing. I will revert the changes made to our forks and check again, which unit tests caused which deprecations to appear in detail. If these tests contain valid cases, I will provide a more polished pull request instead of this sledgehammer solution within the next days. :D
Same for this pull request on doctrine1.
Both pull requests can be closed, thanks for the input!

@connorhu
Copy link
Collaborator

When I finish migrating the test system to phpunit we will see much more clearly.

@sebsnake sebsnake closed this Sep 20, 2023
@sebsnake sebsnake deleted the master_fos1_php81 branch September 20, 2023 11:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants