Received a fatal because mb_substr was not found.

[DragonBe]

Hi,

I received a fatal error because multibyte support was not enabled and because the application was looking for mb_string in namespace Facebook\PseudoRandomString it wasn't able to find it.

Aparently you only get the notification about the mbstring extension when you install through composer (which we didn't do because of our architecture).

composer install
Problem 1
    - The requested PHP extension ext-mbstring * is missing from your system. Install or enable PHP's mbstring extension.

Simple solution is to validate that the module is loaded in code and throw an exception if not (might break BC for everyone not using composer install. Even though this is a BC, I rather have an exception that's captured at the top-level than a Fatal killing all processes.

if(1 !== extension_loaded('mbstring')) {
    throw new \RuntimeException('Multibyte support required');
}

Then load the function with proper \ root reference.

return \mb_substr(\bin2hex($binaryData), 0, $length);

[SammyK]

Hey @DragonBe! Good to see you in the open source world. :)

I totally thought we killed the mbstring requirement a while ago since I thought we were only using it for the CSPRNG. And since the bin2hex() will always only generate [0-9a-f], we don't need to worry about supporting > 256 bits.

But I did some searching and another place it's used is in SignedRequest to validate the signed request signature which also doesn't need > 256 bit support.

So in those two cases we could just change all references to mb_substr() to substr().

The only place we do need mbstring is in the cURL HTTP Client (ew) but you can easily get around that by defining the stream HTTP client option when you instantiate the \Facebook\Facebook service to avoid using the cURL client.

# Use the PHP stream implementation
$fb = new \Facebook\Facebook([
    'http_client_handler' => 'stream',
]);

So we should really update PseudoRandomStringGeneratorTrait & SignedRequest to use substr() and move the "ext-mbstring": "*" requirement in composer.json to the suggest option and explain that if you're using the cURL HTTP client you'll need mbstring installed.

Do you want to take care of that PR or you want me to? :)

PS: Are you going to be at Lone Star or php|tek this year? :)

[SammyK]

PPS: We should probably default the HTTP client to the stream implementation instead of cURL so that people who don't have mbstring installed can use the SDK out of the box without further configuration. Plus the stream client is less prone to bugginess than cURL. What are your thoughts @yguedidi?

[yguedidi]

@SammyK changing the default HTTP client to stream is a BC break.
I think we can find a better way to split the raw response, something like:

$parts = explode("\r\n\r\n", $rawResponse);
$body = array_pop($parts);
$headers = array_pop($parts);

Then, bye bye mbstring dependency :)

[SammyK]

👍 Let's do it! :)

[yguedidi]

I let you do it, I don't have the time :(

[SammyK]

No worries! I can to it but wanted to offer @DragonBe the honors first if he wanted to take it. :)

[DragonBe]

Hey @SammyK, I know you were around on this SDK as you've mentioned it a gazillion times during your Roundtable podcasts. Good to see you here.

I didn't had much time to dig into the whole project, it was tossed over to me because a developer had issues integrating this SDK into a ZF1 project. And of course, we bake our servers from source, mbstring was not an extension that we included. No biggy, just updating the configure run and go… but I did find it odd that when such an extension is used there was no checking if it existed. Of course you can move those hard requirements in the application bootstrapper but that would have a huge impact on the users, while this change I made only has an impact for people like me who don't use Composer and don't read the documentation (otherwise I'd known).

The benefit of this PR is that I had you guys look at some legacy functionality that you might want to upgrade. I prefer to use the PSR-7 approach and use middleware to connect to the outside world, but your stream handler looks just as great.

Unfortunately I don't have the time yet to work this one out so I'll pass on your offer. Sorry.

In regards to conferences, I am going to SunshinePHP and I submitted to TEK. This year is really busy so I won't do as many conferences as I did last year. But we'll get to see each other somewhere on this planet 😸

[SammyK]

Haha. I didn't realize I mentioned that so many times. A gazillion! That's a lot! :D

No worries - I'll try to punch this PR through sometime this week if I get a chance.

I was going to SunshinePHP but I had some things come up last minute. :/ Hopefully I'll see you at tek if you end up going! :)

[gfosco]

Merged #548