Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Notice: iconv_strpos(): Detected an illegal character in input string #5

Closed
wants to merge 2 commits into from

Conversation

wucdbm
Copy link
Contributor

@wucdbm wucdbm commented Apr 12, 2022

This is breaking, PHP needs to be bumped to >= 8.0

@wucdbm
Copy link
Contributor Author

wucdbm commented Apr 12, 2022

So the story goes like this, was wondering why things aren't being cached, turns out the cache had failed silently.
The PR title is the error checking if the string starts with this string using mb_strpos

Additionally, the conditional checker was wonky, mb_strlen always seemed to return 0, so it's now just \strlen

@b1rdex
Copy link
Owner

b1rdex commented Apr 12, 2022

Can you please provide a regression test case?

AFAIK, mb_* with US-ASCII encoding were used because str_* functions may be replaced with corresponding functions from mbstring in runtime (https://www.php.net/manual/en/mbstring.configuration.php#ini.mbstring.func-overload).

I see the feature is removed in PHP 8, so I assume we're safe to get rid of mbstring.

@wucdbm
Copy link
Contributor Author

wucdbm commented Apr 12, 2022

Will do. Actually, we'll need to upgrade phpunit as well.
By the way, what's up with the weird test namespace namespace Sp\Tests\PredisCompress

@b1rdex
Copy link
Owner

b1rdex commented Apr 12, 2022

Wow, nice finding. It should be fixed too 😬

@wucdbm
Copy link
Contributor Author

wucdbm commented Apr 12, 2022

Another update: So I just had to install ext-mbstring on the system due to PHPUnit 9.5, and this magically started working and it (as in the old code) keeps working even after I removed mbstring and the system library it installed.

Anyhow, I think we should take this path instead and ensure it's stable, because when it fails, it does so silently. I'll create a test for the GzipCompressor, but we should ensure it runs in an environment that does not have ext-mbstring, which we can't do because PHPUnit requires that.

Hmpf?

@b1rdex
Copy link
Owner

b1rdex commented Apr 13, 2022

Ok, I see the issue. From the original title Notice: iconv_strpos(): Detected an illegal character in input string it looks like you have something in your env that replaces mb_strpos with iconv_strpos.

If I get you right, installing mbstring fixes the issue. That's fine for me.

I'm planing to release a new php 8+ version with no mbstring requirement.

@b1rdex
Copy link
Owner

b1rdex commented Apr 13, 2022

Well, actually, I'll release PHP 7.2+ version because mbstring.func_overload was removed in that version.
Incorrect. It was deprecated in 7.2 and removed in 8.0.

@b1rdex
Copy link
Owner

b1rdex commented Apr 13, 2022

Please try the new PR: #6

@b1rdex b1rdex closed this Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants