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

Add mbstring requirement to composer.json (#109) #110

Closed
wants to merge 1 commit into from
Closed

Add mbstring requirement to composer.json (#109) #110

wants to merge 1 commit into from

Conversation

colinodell
Copy link
Member

See issue #109

@jasverix
Copy link
Contributor

I actually don't think packages such as this should requre extensions - because some people would rather use polyfills such as symfony/polyfill-mbstring. But maybe as a suggestion, so the message will pop up when installing this package.

@garrettw
Copy link

garrettw commented Jul 3, 2017

Maybe what composer really needs is a way to require one thing OR the other, which would also need the ability to mark one of the options as preferred, so that in the case that the user has neither, composer will know what to install.

@duncan3dc
Copy link
Member

Doesn't provide already do that? Maybe it just needs extending to be able to "provide" PHP extensions. Then symfony/polyfill could "provide" ext-mbstring

@garrettw
Copy link

garrettw commented Jul 3, 2017

Actually @colinodell I'm wondering if it would be better to just require the polyfill and not the extension. That way if they have the extension it will just use that, and if not, it will use the polyfill. Seems to me that doing it that way would be best for ease of use. Some other packages do it that way.

@duncan3dc
Copy link
Member

Rather than install the polyfill every time, I've added a suggest (a5babd1). Thanks to the nice way composer works it will only suggest it if it detects ext-mbstring is not installed

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.

None yet

4 participants