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

Remove zend-stdlib dependency #1284

Merged
merged 1 commit into from
Feb 17, 2018
Merged

Remove zend-stdlib dependency #1284

merged 1 commit into from
Feb 17, 2018

Conversation

Trainmaster
Copy link
Contributor

Description

Fixes #1256

Checklist:

  • I have run composer run-script check --timeout=0 and no errors were reported
  • The new code is covered by unit tests (check build/coverage for coverage report)
  • I have update the documentation to describe the changes

@troosan
Copy link
Contributor

troosan commented Feb 16, 2018

@Trainmaster a better solution would be to use PhpOffice\Common\Text:: isUTF8 instead.
I'll see if can un-deprecate it (@Progi1984?).

@Progi1984
Copy link
Member

👌 with that.

@Trainmaster
Copy link
Contributor Author

@troosan I changed it to PhpOffice\Common\Text:: isUTF8.

@troosan troosan added this to the v0.15.0 milestone Feb 17, 2018
@troosan troosan merged commit 7324070 into PHPOffice:develop Feb 17, 2018
@Trainmaster Trainmaster deleted the feature-remove-zend-stdlib-dependency branch February 17, 2018 08:26
@JamesB7
Copy link

JamesB7 commented Feb 17, 2018

I think the is_string check is important: preg_match will return true for non-strings, such as integers. (Yes, this surprised me too. Hurrah for PHP?)

Without the is_string check, callers of isUTF8 (such as toUTF8) may pass through non-string values without converting them to string (such as utf8_encode does).

@troosan
Copy link
Contributor

troosan commented Feb 17, 2018

I create a PR in PHPOffice/Common#19 to modify the method

@Progi1984
Copy link
Member

Merged ;)

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.

4 participants