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

Update Id3v2.php #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update Id3v2.php #19

wants to merge 4 commits into from

Conversation

za-ek
Copy link

@za-ek za-ek commented Nov 3, 2022

utf8_encode corrupts cp1251 strings

utf8_encode corrupts cp1251 strings
@duncan3dc
Copy link
Owner

Hi @za-ek, thanks for your PR but I'm afraid this breaks the functionality of the library (if you run the unit tests you'll see Id3v2Test::testUtf16WithoutBom fails).

Please can you provide a unit test demonstrating the issue you're experiencing, then we can find the best way to fix it without introducing any regressions

@za-ek
Copy link
Author

za-ek commented Nov 3, 2022

ah sorry I didn't see there is a test folder. Do you mean I make a PR with a test?

@duncan3dc
Copy link
Owner

Do you mean I make a PR with a test?

You can just push it to this PR, or if you're struggling with a test just any script/snippet I can use to reproduce the problem you're trying to fix

@za-ek
Copy link
Author

za-ek commented Nov 3, 2022

I add mp3 file and test so now you can reproduce the problem

@za-ek
Copy link
Author

za-ek commented Nov 3, 2022

btw you can add this line after line 166 in Id3v2.php file to fix test you mentioned:

$value = Bom::removeBom($value);

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