Skip to content

Conversation

@fredden
Copy link
Contributor

@fredden fredden commented Dec 23, 2022

WHY are these changes introduced?

On line 188 of src/Clients/Http.php, the function mb_strlen() is used. After installing this library on a new server, I got an error from PHP saying:
PHP Fatal error: Uncaught Error: Call to undefined function Shopify\Clients\mb_strlen() in .../vendor/shopify/shopify-api/src/Clients/Http.php:188

->withHeader(HttpHeaders::CONTENT_LENGTH, mb_strlen($bodyString));

WHAT is this pull request doing?

This pull request updates the list of requirements in composer.json to include the mbstring extension, which is required for this library to work properly.

Update: while I was completing the 'checklist' in the GitHub pull request template, I added a test to catch this problem, and found some more extensions which are required and not listed.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above
  • I have added/updated tests for this change
  • I have updated the documentation for public APIs from the library (if applicable)

mb_strlen() is used on line 188 of src/Clients/Http.php
ctype_digit() is used on line 113 of src/Utils.php
hash_equals() is used on line 78 of src/Utils.php
hash_hmac() is used on line 76 of src/Utils.php and in src/Auth/OAuth.php (lines 269, 294)
@wilsenhc
Copy link

LGTM! I hope we can get this merged soon

@wilsenhc
Copy link

Still LGTM, hopefully we can get this merged soon @paulomarg 🚀

@fredden fredden mentioned this pull request Apr 27, 2023
3 tasks
@Jan0707
Copy link
Contributor

Jan0707 commented Apr 28, 2023

Hi @fredden !

We have just recently added validation and normalization for composer.json to our workflow. Would you mind updating your PR to resolve the conflicts :)

Looks good otherwise!

@fredden
Copy link
Contributor Author

fredden commented Apr 28, 2023

@Jan0707 yes. I've resolved the merge conflicts introduced today twice now. Feel free to squash-merge this to keep the main branch clean, or I can rebase this onto the main branch if you'd prefer.

Copy link
Contributor

@Jan0707 Jan0707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@Jan0707 Jan0707 merged commit 5063bd6 into Shopify:main Apr 28, 2023
@fredden fredden deleted the mbstring branch April 28, 2023 13:43
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.

3 participants