Nexmo / nexmo-laravel

Add Vonage functionality such as SMS and voice calling to your Laravel app with this Laravel Service Provider.
MIT License
317 stars 79 forks source link

@new Verification returns an exception #57

Closed gothiquo closed 3 years ago

gothiquo commented 3 years ago

Expected Behavior

I was looking and debugging the code after having an exception, then suddenly I saw you try to hide the errors using the "@" symbol; first, that is an evil and dangerous practice, and also that is not a solution (like in my case).

protected function createVerificationFromArray($array): Verification
    {
        if (!is_array($array)) {
            throw new RuntimeException('verification must implement `' . VerificationInterface::class .
                '` or be an array`');
        }

        foreach (['number', 'brand'] as $param) {
            if (!isset($array[$param])) {
                throw new InvalidArgumentException('missing expected key `' . $param . '`');
            }
        }

        $number = $array['number'];
        $brand = $array['brand'];

        unset($array['number'], $array['brand']);

        return @new Verification($number, $brand, $array);
    }

Current Behavior

Possible Solution

The error thrown in the Verification class should be removed, or the https://github.com/Nexmo/nexmo-php/blob/master/src/Verify/Client.php class should be modified (the start function).

Steps to Reproduce (for bugs)

I tried to verify the user's phone number as follow:

Nexmo::verify()->start(new Request('hier_the_phone_number', 'MyApp'))

Then the exception ArgumentCountError is shown. The problem is the function start in the class /Verify/Client.php.

Context

I want to verify the user's phone number after its registration, but currently, it is not possible.

Your Environment

I am using laravel sail.

dragonmantank commented 3 years ago

Normally, yes, @ error suppression is a bad thing. In this case it's done to cut down on the amount of deprecation errors that are thrown, and the error suppression will be removed in v3. In v3 the start() method will not be nearly as open-ended as it currently is and will require a Vonage\Verify\Request object.

That being said, passing a Vonage\Verify\Request object into start() should not generate any errors at all, as it has all the information necessary to create the internal objects and make the HTTP request to us. Can you provide the full stack trace of the error you are getting?

gothiquo commented 3 years ago

Thanks for the answer @dragonmantank ! Here is my call: $verification = Nexmo::verify()->start(new Request($phone, 'Mealthi')); And then here the exception thrown: image

As I mentioned above, the problem is happening in the class /Verify/Client.php

dragonmantank commented 3 years ago

Sorry for the bit of the delay, New Years and all.

Are you running PHP 8? I know the error handler signature changed between 7 and 8, and we recently pushed a fix to handle that around the PHP 8 release date. Can you try upgrading the packages to see if that fixes it? PHP 8 support was added in 2.7.0 of our core library. You can check which version of our library you are using with:

composer info vonage/client-core or composer info nexmo/client-core

If the library is less than 2.7.0, you should be able to update it with:

composer update nexmo/laravel --with-dependencies

should work. You may need to try

composer update nexmo/laravel --with-all-dependencies

gothiquo commented 3 years ago

Hi @dragonmantank ! Happy New Year! I tried to info command to see the version: 2.6.0 So I called composer update nexmo/laravel --with-dependencies and I got the following error:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - tymon/jwt-auth is locked to version 1.0.2 and an update of this package was not requested.
    - tymon/jwt-auth 1.0.2 requires php ^5.5.9|^7.0 -> your php version (8.0.0) does not satisfy that requirement.

And then the next command composer update nexmo/laravel --with-all-dependencies:

 Problem 1
    - Root composer.json requires laravel/framework ^8.12, found laravel/framework[v8.12.0, ..., 8.x-dev] but these were not loaded, likely because it conflicts with another require.
  Problem 2
    - laravel/tinker is locked to version v2.5.0 and an update of this package was not requested.
    - laravel/tinker v2.5.0 requires illuminate/console ^6.0|^7.0|^8.0 -> found illuminate/console[v6.0.0, ..., 6.x-dev, v7.0.0, ..., 7.x-dev, v8.0.0, ..., 8.x-dev] but these were not loaded, likely because it conflicts with another require.
  Problem 3
    - tymon/jwt-auth is locked to version 1.0.2 and an update of this package was not requested.
    - tymon/jwt-auth 1.0.2 requires illuminate/auth ^5.2|^6|^7|^8 -> found illuminate/auth[v5.2.0, ..., 5.8.x-dev, v6.0.0, ..., 6.x-dev, v7.0.0, ..., 7.x-dev, v8.0.0, ..., 8.x-dev] but these were not loaded, likely because it conflicts with another require.
  Problem 4
    - facade/flare-client-php 1.3.7 requires illuminate/pipeline ^5.5|^6.0|^7.0|^8.0 -> found illuminate/pipeline[v5.5.0, ..., 5.8.x-dev, v6.0.0, ..., 6.x-dev, v7.0.0, ..., 7.x-dev, v8.0.0, ..., 8.x-dev] but these were not loaded, likely because it conflicts with another require.
    - facade/ignition 2.5.8 requires facade/flare-client-php ^1.3.7 -> satisfiable by facade/flare-client-php[1.3.7].
    - facade/ignition is locked to version 2.5.8 and an update of this package was not requested.

Any suggestion?

Thanks a lot Kind regards

dragonmantank commented 3 years ago

Hello @gothiquo !

your php version (8.0.0) does not satisfy that requirement sums up your issue - you will need to downgrade your version of PHP to some version of PHP 7, or update whatever plugin uses tymon/jwt-auth to something that will pull in a newer version of tymon/jwt-auth that is PHP 8 compatible. This would be some other plugin from ours, as tymon/jwt-auth is not a dependency for any Vonage/Nexmo packages.

gothiquo commented 3 years ago

Thanks a lot ! Problem Solved !

dragonmantank commented 3 years ago

Glad I could help!