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

Make InitialMapper multibyte proof #37

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rvanlaak
Copy link

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5a32bd3 on scribbr:multibyte-initials into 9a54a71 on theiconic:master.

@wyrfel
Copy link
Contributor

wyrfel commented Dec 2, 2019

Hey @rvanlaak . Sorry, been busy - are you still working on this?
I was thinking i'm a bit hesitent to blanco-require the mbstring extension as it's not active by default. I would therefore prefer we added an abstraction file (or two) for all string operations - could be a class or just namespaced functions so we can use the mbstring versions when that extension is loaded and the standard one when not and thereby degrade gracefully.

What do you think?

@rvanlaak
Copy link
Author

rvanlaak commented Dec 2, 2019

Names by their nature simply can be multibyte. Abstracting out the multibyte implementation might cause problems when users don't get that. I would not advise to do that.

This PR is mostly to give you an idea and get your feedback before we continue implementing fixes and adding test names.

@wyrfel
Copy link
Contributor

wyrfel commented Dec 12, 2019

@rvanlaak Please have a look at #39 .
I believe this is a good way of doing this while still providing fallback (inspired by the function abstractions in GuzzleHttp).

@rvanlaak
Copy link
Author

I'm ok with calling these methods if you decide that name-parser isn't going to be multibyte-proof by default 👍 Could you write a not about that in the README?

@wyrfel
Copy link
Contributor

wyrfel commented Dec 12, 2019

Yes, i'm happy to add a note to the README. But i would like to have coverage for all the string functions used in the parser. Do you think you could expand on this and provide a PR that has all the native calls replaced with these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants