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

[Polyfill] mbstring polyfill does not have its iconv dependency declared #52207

Closed
theofidry opened this issue Oct 20, 2023 · 3 comments
Closed

Comments

@theofidry
Copy link
Contributor

Symfony version(s) affected

All versions of symfony/polyfill-mbstring

Description

From the description of Mbstring:

Partial mbstring implementation in PHP, iconv based, UTF-8 centric.

And indeed the class uses a few ext-iconv functions, for instance: https://github.com/symfony/polyfill-mbstring/blob/1.x/Mbstring.php#L116

The package is quite old (~8yo) so I do not know if this was a conscious decision at that time, a lack of support from Composer or if this is actually wanted. But my understanding is that the package currently depends on ext-iconv but does not declare it. As a result you can end up with the following failure:

Uncaught Error: Call to undefined function Symfony\Po  
  lyfill\Mbstring\iconv_substr() in /path/to/project/vendor/symfony/  
  polyfill-mbstring/Mbstring.php:660

How to reproduce

I did not try to reproduce it on its own as I think the issue is understandable enough. But I think currently this can be done by:

  • having an project using GitHub Actions with a ubuntu-latest image
  • using the PHP binary coming with the image
  • having the project using symfony/polyfill-mbstring and using the mb_substr() function

I encountered this issue while debugging something entirely unrelated in https://github.com/box-project/box/actions/runs/6591562359/job/17910413036?pr=1089.

Possible Solution

Add ext-iconv to the require of the composer.json

Additional Context

No response

@theofidry theofidry added the Bug label Oct 20, 2023
@theofidry theofidry changed the title [Polyfill] mbstring polyfill dependency [Polyfill] mbstring polyfill does not have its iconv dependency declared Oct 20, 2023
@nicolas-grekas
Copy link
Member

This is (was) on purpose, because there's also a polyfill for iconv.
Not listing the iconv polyfill is also on purpose, because it's a heavy one.
See composer/composer#11669 for possible next steps.
Nothing we can do here (also because it's the wrong repo ;) )

@nicolas-grekas
Copy link
Member

For box, you might want to require the iconv-polyfill.

@theofidry
Copy link
Contributor Author

For box, you might want to require the iconv-polyfill.

Yeah I mean it's not a problem in itself, I just wanted to make sure it was not an issue that just went under the radar for whatever reason.

theofidry added a commit to theofidry/box that referenced this issue Oct 20, 2023
- `ext-mbstring` is required and to allow it to work as a PHAR the
  polyfill is added.
- `symfony/polyfill-mbstring` require `iconv` (see
  symfony/symfony#52207).
theofidry added a commit to box-project/box that referenced this issue Oct 21, 2023
- `ext-mbstring` is required and to allow it to work as a PHAR the
  polyfill is added.
- `symfony/polyfill-mbstring` require `iconv` (see
  symfony/symfony#52207).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants