J7mbo / twitter-api-php

The simplest PHP Wrapper for Twitter API v1.1 calls
MIT License
1.82k stars 802 forks source link

Error handling #247

Closed paulcanning closed 6 years ago

paulcanning commented 6 years ago

How would I go about adding error handling into this?

Let's say one of the keys is not provided, and you cannot connect to the API, you get an error object back.

But I'm struggling to add logic into my code that lets me handle the error and spit out a nice message on the frontend of my site.

J7mbo commented 6 years ago

There does need to be better handling.

Also, this is just a raw library that wraps curl. It doesn't have any nice methods like tweetWithImage(int $imageId) which requires you to pass in a valid image id.

You can easily create your own object which decorates this one and enforces custom validation and constraints.

paulcanning commented 6 years ago

Is there a way I can wrap any of this in a try/catch?

J7mbo commented 6 years ago

You can enforce the keys being provided at the object level for a start (never instantiate an object in an invalid state). Let's say we're updating a status, let's look at the docs.

For example, we can see that $status is required, but there's also: $in_reply_to_status_id. So you could do this:

public function tweet(string $status) {
    if (strlen($status) > 140) {
        throw new TweetIsTooLong(strlen($status));
   }

   $this->exchange->etc // Tweet here
}

... and then you catch the exception and show a nice error message to the user, although really you should be validating the length on the client-side too, so really it's an arbitrary example.

Then, if you want to reply to a tweet...

public function tweetReplyTo(string $status, int $replyStatusId) {
    // Same validation for status
    // If you want to validate that a status actually exists, try and get the status first and check that it exists. However, you should really only be showing valid tweets in the front-end and passing the correct id back through, so this validation should already exist. Throw an exception if it doesn't.
}

The responses from twitter aren't very helpful but the above does what you want...

paulcanning commented 6 years ago

How do i force the keys to be required, when I am letting the user define them?

This is what my original post was about - if a key is missing, how am I best dealing with that scenario?

J7mbo commented 6 years ago

When you say keys, are you referring to the authentication tokens? Or array keys required to make the API work, like a status key / value pair?

paulcanning commented 6 years ago

The authentication tokens. I'm letting the user put these in, so it maybe that somehow one is missing (yes, I know I could make them required etc).

It was about that, and then a more general how to deal with errors, and I'm struggling to make any try/catch statements work (not sure where they should be placed etc)

J7mbo commented 6 years ago

If this is just a generic PHP problem with understanding how to utilise exception handlers and try / catches, then I can try and help but you may get faster responses from StackOverflow.

However if this is specific to this library... then the problem isn't that you need to enforce the users providing them.. there are four different keys / tokens. They must all be provided. You can enforce this in the front end by not submitting data to the server unless all four fields are filled and look like valid tokens. Then you also enforce this on the back end by making sure exactly four variables are passed in via POST.

Your main problem is ensuring that these tokens work. You can test this by trying a simple GET request (this is tested in the library, see tests/) so if yours doesn't work then the tokens are incorrect. If you get a response from the exchange containing "not authorised" or the exact string that it is, you throw an exception which you then catch and show a nice message to the user telling them to sort their tokens out.

Is that helpful?

paulcanning commented 6 years ago

Ok, so here is my code:

Twitter class

class Twitter_Feed {

    /**
     * OAuth settings
     *
     * @var array
     */
    private $settings;

    /**
     * The API response
     *
     * @var string
     */
    private $response;

    /**
     * Twitter_Feed constructor.
     */
    public function __construct() {
        $this->settings = [
            'oauth_access_token' => get_option( 'twitter_oauth_token' ),
            'oauth_access_token_secret' => get_option( 'twitter_oauth_token_secret' ),
            'consumer_key' => get_option( 'twitter_consumer_key' ),
            'consumer_secret' => '', // purposefuly missing
        ];
    }

    /**
     * Get the feed
     */
    private function get_feed() {
        $url = 'https://api.twitter.com/1.1/statuses/user_timeline.json';
        $get_field = '?screen_name=<MYNAME>&count=3&trim_user=true&exclude_replies=true';
        $request_method = 'GET';

        $twitter = new \TwitterAPIExchange( $this->settings );
        $this->response = $twitter->setGetfield( $get_field )->buildOauth( $url, $request_method )->performRequest();

        return $this->response;
    }

    /**
     * Output feed
     *
     * @return array|mixed|object
     */
    public function output_feed() {
        $this->get_feed();

        return json_decode( $this->response );
    }
}

Plugin code:

function twitter_init_settings() {
    return new Twitter_Settings();
}

twitter_init_settings();

Template code:

$tweets = twitter_feed()->output_feed();

foreach ( $tweets as $tweet ) {
    echo $tweet->text;
}

So, with a missing key, the response from twitter is an error message. But because I am trying to spit out $tweet->text I get Notice: Trying to get property of non-object on the frontend.

If I want to catch the error in the get_feed method, how can I stop the rest of the code executing and spit out an error message (or nothing, and log the error?)

J7mbo commented 6 years ago

Firstly you need to make sure in your constructor that if any of the get_option()s are empty, you throw an exception.

J7mbo commented 6 years ago

Also with the property of non-object, you can easily do a if (!isset($tweet->text)) { /** Throw an exception here **/} - if the credentials are valid, you will get a tweet back. But then you have many problems - you won't be able to tell the difference between tweets existing (which they may not) or invalid credentials. So you'd need to perform a simple GET request on some tweets that are guaranteed to exist first to ensure the credentials work. Then if it fails in the next step it's because there aren't any tweets to be displayed - you can throw individual exceptions accordingly.

paulcanning commented 6 years ago

I tried that, but I get an uncaught exception error.

in the class file

public function __construct() {
   throw new Exception(); // just for testing
}

in the plugin file

function twitter_init_settings() {
    try {
        return new Twitter_Settings();
    } catch (Exception $e) {
        echo $e->getMessage();
    }
}
J7mbo commented 6 years ago

Try using \Exception --- the Exception object is in the global namespace.

paulcanning commented 6 years ago

Sorry, it is, small typo, but i do have it as that

J7mbo commented 6 years ago

This plugin file looks like it's wordpress or something? I can't help with third party solutions, but it's obvious that throwing an \Exception in a constructor is caught by PHP outside of it. I'm not sure how else I can help, seems like a problem with your third party library (wordpress?)

PS your code says twitter_feed(), but that's a function call. Your Twitter_Feed object is a class, and I don't see new Twitter_Feed anywhere...

paulcanning commented 6 years ago

Hmm, balls! Yes, it's WordPress, but I'd have assumed throwing a normal PHP exception would be fine :(

J7mbo commented 6 years ago

I'd highly recommend getting this thing working entirely outside of wordpress first. It's exactly the same with frameworks. Build your solution as agnostic as possible.

As soon as you get it working, then your only job is to 'slot' it in, and that's the easiest bit.

Once you get to the slotting it in stage, your first thing to figure out is absolutely nothing to do with this library, or code. You want to figure out why throwing a simple exception isn't caught. No other code should be used, just an exception. Solve it step by step

paulcanning commented 6 years ago

Agreed. It looks like WP deals with exceptions in an odd way :(

J7mbo commented 6 years ago

I'm going to close this as a third-party (wordpress-specific) problem, but if you need any help directly with the library then please feel free to re-open / create another issue.