Shopify / shopify-api-php

MIT License
376 stars 172 forks source link

Improve "Private App" API usage #151

Open MikeParkin opened 2 years ago

MikeParkin commented 2 years ago

Overview/summary

Currently using this library when connecting to a private app is confusing.

It takes quite a long time to work out what you are doing, when it feels like it could be really simple.

Motivation

In our usecase, we only want to consume the Shopify admin api via REST, with a private app access token.

Currently to do that you have to have the following code:

Context::initialize(
  'xxx',                                          // Not used
  'yyy',                                          // Not used
  'read_orders, write_orders',                    // Not used
  'http://localhost',                             // Not used
  new FileSessionStorage('/tmp/php_sessions'),    // Not used
  'latest',
  false,                                          // Not used
  false,                                          // Needs to (incorrectly) be set to false
);

$rest = new Rest($domain, $storeApiToken);

This is not helped by the fact that there is a bug on this line:

https://github.com/Shopify/shopify-php-api/blob/main/src/Clients/Rest.php#L46

So you actually have to Context::initialize with "privateApp" to to false, so it uses the access token not the secret key.

       $headers[HttpHeaders::X_SHOPIFY_ACCESS_TOKEN] =
            Context::$IS_PRIVATE_APP ? Context::$API_SECRET_KEY : $this->accessToken;

There is barely any point in having to call Context::initialize, the only reason for doing it is to:

In an ideal situation I would just need to do this:

$rest = new Rest($domain, $storeApiToken);

Possible Improvements

Happy to contribute these changes, if you are welcome to receive them.

schmoove commented 2 years ago

Thanks for this – the docs could use a lot of improvement (notably example PHP code for different app types)

MikeParkin commented 2 years ago

Ah, I can understand now why this library works the way that it does. It's following the upstream Ruby version of the same API library:

https://github.com/Shopify/shopify_api#steps-to-use-the-gem

From what I can understand this appears to have the same issue.

The Ruby API version 9 had better documentation explaining how to use the library for private apps:

https://github.com/Shopify/shopify_api/tree/v9#2a-private-apps https://github.com/Shopify/shopify_api/tree/v9#6a-making-requests-to-the-graphql-api

Going to make an issue request over there to find out if this is intentional or something they plan to fix too...

MikeParkin commented 2 years ago

On further reading, I am guessing I am slightly "mis-using" the intended API here. It appears the expected behaviour is to create a "session" using Context::initialize, then use the Rest library and pass back in the shop and access token from the session I just made. This does seem a bit overly complex for my use case though!

https://github.com/Shopify/shopify-php-api/blob/main/docs/usage/rest.md#rest-client

use Shopify\Clients\Rest;

$client = new Rest($session->getShop(), $session->getAccessToken());
$response = $client->get('products');

Created a ticket here https://github.com/Shopify/shopify_api/issues/911

MikeParkin commented 2 years ago

Just reading through the issues, it appears there are a number of duplicates on this, all relating to the initialisation / getting started:

https://github.com/Shopify/shopify-php-api/issues/100 https://github.com/Shopify/shopify-php-api/issues/104 https://github.com/Shopify/shopify-php-api/issues/120 https://github.com/Shopify/shopify-php-api/issues/176

lukeholder commented 2 years ago

This is still an issue that is not just documentation related. Like @MikeParkin I also believe there is a bug in the code:

https://github.com/Shopify/shopify-php-api/blob/main/src/Clients/Rest.php#L46

The Rest Client can not make a HTTP API request when the context has been set to a private app (isPrivateApp set to true) with an "Admin API access token".

A private app is more likely to use an "Admin API access token" that was not obtained through an OAuth created session, and thus does not require the referenced context to be configured anyway.

CleanShot 2022-05-31 at 17 41 36@2x

Admin API access tokens should able to be used with the Rest class while the context's isPrivateApp is set to true, or the whole condition should be removed.

I don't even think one can use an "API Secret Key" as the X-Shopify-Access-Token header to make a direct request to the API anyways - so that is also a bug yeah? If you can, please link the docs on it? Cant find anything about using a API Secret Key to make a API request anywhere here: https://shopify.dev/api/admin-rest

Happy to make a PR, but being new the to lib and the API, I am hoping I am not missing something?

lukeholder commented 2 years ago

I am able to do this as a workaround:

        // initialize the context

        $session = new Session(
            id:'NA',
            shop: $hostName,
            isOnline: false,
            state:'NA'
        );
        $session->setAccessToken($accessToken);

        $products = Product::all($session);

That is with isPrivateApp: false

hparadiz commented 2 years ago

Why is Context::initialize a singleton anyway? I might want to communicate with multiple stores in the same PHP thread.

Zelfapp commented 2 years ago

@lukeholder has the most direct workaround. Docs are severely lacking for private apps with a permanent access token and just in general.

This works for private app with permanent access token.

use Shopify\Auth\FileSessionStorage;
use Shopify\Auth\Session;
use Shopify\Rest\Admin2022_07\Customer;

Shopify\Context::initialize(
    'key',
    'secret',
    ['read_customers'],
   'shop',
    new FileSessionStorage('your_apps_php_sess_save_path'),
    '2022-07',
    false,
    false,
);

$session = new Session(
    id:'NA',
    shop: 'shop_domain',
    isOnline: false,
    state:'NA'
);
$session->setAccessToken($_ENV['PERMANENT_ACCESS_TOKEN']);

$customer = Customer::search(
    $session,
    [],
    ['query' => 'email:customer@shop.com'],
);
github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 90 days with no activity. It will be closed if no further action occurs in 14 days.

lukeholder commented 1 year ago

This should not be closed.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

MikeParkin commented 1 year ago

Still shouldn’t close this.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

lukeholder commented 1 year ago

Still shouldn't close this.

flogaribal commented 1 year ago

Hi Everyone!

Thank you all for this issue, it helped me understanding better how to use this package for private app I agree with you all, it should be way simpler/easier & clearer to use seeing all the parameters we have to specified that are not used...

Open to help on this one! (first time helper here)

MikeWillis commented 1 year ago

Thanks to all who contributed here, particularly @lukeholder and @Zelfapp - your fixes rescued me from 3+ hours of struggling. I'd been reading all the documentation thinking I must be missing something, because there seemed to be no explanation whatsoever for how to use the api for private apps. Glad it wasn't just me.

elburro1887 commented 1 year ago

THANK YOU SO MUCH EVERYONE!!

I was struggling for 2 hours here trying to figure out how I could use "Access Token / Basic AUTH" to connect to my Shopify Private App created from the admin (NOT PARTNER CENTER APP), as described here: https://shopify.dev/docs/api/usage/authentication

How can the documentation be so bad?? There is no simple example anywhere here, the docs are spread over dozens of confusing pages and I simply couldn't figure it out. Seems like I am not the only one though!!

Also, the sample app shopify-app-template-php is a Laravel/React/Nodejs monstrosity that I'm sure it could be useful, but for someone just starting out and trying to make simple API calls, how should anyone be able to figure anything out using that without wasting hours?

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

lukeholder commented 1 year ago

Still an issue with the packages PHP API

ryanm-bw commented 1 year ago

Posting this here in lieu of the docs ever being updated (since this is now the second time I've run into this issue and wound up back here trying to solve it). Here's the complete code for using the REST API:

<?php
require 'vendor/autoload.php';

use Shopify\Clients\Rest;
use Shopify\Context;
use Shopify\Auth\FileSessionStorage;

define('API_KEY', '.............................................');
define('API_SECRET', '.....................................................');
define('ACCESS_TOKEN', '............................................');
define('SHOP', '...............................myshopify.com');

Context::initialize(
    apiKey: API_KEY,
    apiSecretKey: API_SECRET,
    scopes: 'read_themes, write_themes',
    hostName: 'http://127.0.0.1:4000',
    sessionStorage: new FileSessionStorage('/tmp/php_sessions'),
    apiVersion: '2023-04',
    isEmbeddedApp: false,
    isPrivateApp: false,
);

$client = new Rest(SHOP, ACCESS_TOKEN);
$response = $client->get('themes');
github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

lukeholder commented 1 year ago

Still an issue with the packages PHP API

github-actions[bot] commented 11 months ago

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

elburro1887 commented 11 months ago

Issue is still not resolved.

Also: GitHub stale bot considered harmful

ZacharyDuBois commented 11 months ago

I'd also agree, using this SDK for any app that doesn't "live in Shopify's admin" is extremely painful. This SDK makes assumptions that you are not using a framework (this SDK tries to handle sessions, key management, etc for you) and generally is trying to do too much.

The Context static class is killer if you are writing some maintenance tasks that interact with more than one Shopify store since it's statically defined. When Stripe moved to the static config helper, they still allowed you to use the underlying components giving you the ability to completely ignore it. They also allowed for more than one instance of the StripeClient to exist with multiple different configs ($clientA could be configured with Guzzle as the Http provider and tied to account A and $clientB could be set up with a Curl provider and tied to account B). The way Shopify implemented this library, only one Context can exist at a time hindering doing anything in parallel or batched across stores.

For admin/develop apps, this SDK makes you set up so much around supporting multiple stores when all you really need is the Rest class with an API key. The requirement for a session provider is also annoying since most frameworks will handle this. Just document what needs to be stored (normally, the typical OAuth tokens, etc). All of this causes duplicated code/effort and either nested session providers or two session providers. Same with the webhook handler. Most frameworks have eventing, etc already built in. This SDK is creating an issue of two event busses existing within one app. They use the Yii framework in a lot of their references. Even Yii supports everything they are reimplementing in this library.

I honestly feel a re-write or simplified ShopifyClient is needed. Almost worth stripping out Rest and Graphql to their own packages and make this SDK a "simplified" version for simple apps.

Side note: I do appreciate the ability (like many SDKs) to override the Http client being used to a different PSR compatible API client. In my case, I inject one that has some Guzzle middleware to log all requests and responses for debugging. But this could, again, be defined when constructing a new instance of Rest.

My comments here are very opinionated but Shopify is the monolith storefront. Having a monolith library is very annoying when you need to interact with one part.

EDIT:

It appears this complaint is across the board with the Shopify SDKs. Similar thread on the Ruby side of things: https://github.com/Shopify/shopify-api-ruby/issues/911#issuecomment-1144624204

ZacharyDuBois commented 9 months ago

Are we going to get an update on it? This SDK is useless for most implementations. The API client classes should be extracted from the OAuth, SessionStorage, etc classes. In reality this should be maybe 4 or 5 different packages. One for Rest, GraphQL, OAuth, Utils, and a broiler plate for an app where you can put the basic functionality of the Context class.

They are asking for the same thing on the Ruby side. This SDK is unusable. You cannot even instantiate the Rest class and use it without Context being initialized since it depends on the HTTP_CLIENT_FACTORY and you cannot freely inject it outside of Context. And since this Context object is required by almost every class, you can't just extend the Rest class and remove the references to it since the base Http class also makes use of it. It makes no sense other than making an unmaintainable mess.

github-actions[bot] commented 7 months ago

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

ZacharyDuBois commented 7 months ago

Definitely not stale and stale bot is useless.

lukeholder commented 6 months ago

Not stale.

benblub commented 5 months ago

Some simple docs would be nice^^ Thanks for this thread 👍

github-actions[bot] commented 3 months ago

This issue is stale because it has been open for 60 days with no activity. It will be closed if no further action occurs in 14 days.

MikeWillis commented 3 months ago

Not stale

fulldecent commented 3 months ago

I was able to access the Admin REST API using this minimal implementation (workaround)

<?php

namespace App\Controllers;

use Shopify\Clients\Rest;
use Shopify\Context;
use Shopify\Auth\FileSessionStorage;

Context::initialize(
    apiKey: 'your_mom',
    apiSecretKey: 'your_mom',
    scopes: 'your_mom',
    hostName: 'your_mom',
    sessionStorage: new FileSessionStorage('/tmp/your_mom'), # /dev/null does not work here
    apiVersion: '2024-04',
    isEmbeddedApp: false,
    isPrivateApp: false,
);

$client = new Rest($_ENV['SHOPIFY_SHOP'], $_ENV['SHOPIFY_ADMIN_API_ACCESS_TOKEN']);
$response = $restResponse->getDecodedBody();

Please note that your_mom above is a literal value. It does not need to be replaced by some other value. You are welcome to also use this API key I'm "using".

ZacharyDuBois commented 3 months ago

The issue is that method will just break with any background update. The dependency on this Context class needs to be removed entirely.

fulldecent commented 3 months ago

Yes, for sure. And that's what the Stateless in REST means.

alexwhb commented 2 months ago

It's crazy how long this issue has been open. I'm running into the same issue. Thinking maybe it'd be prudent to just write my own graphQL access using Guzzle client or something.

paulomarg commented 1 month ago

Hey everyone. Thank you so much for your patience, and sorry for not responding earlier. The stalebot has been removed from this repo, so that problem is gone.

I agree that there is a bug in that line, it should be using a separate config value for that access token, and not the API key.

We will fix that and take another look at the documentation for private / custom apps to improve it for apps. Thank you all for the suggestions here.

ZacharyDuBois commented 1 month ago

I think the bigger issue is this SDK is dictating how to form your app/service. There is no freedom to throw out the Context class. You can't use the GraphQL or Rest clients without implementing the whole thing. IMHO, this SDK is doing too much. It would be much nicer to see it broken into 4 different libraries (OAuth, Rest, GraphQL, Utilities for views/webhooks) and then if you want the "simple implementation" that this is trying to provide, make that a wrapper for all 4. That way, the advanced users can implement how they want and the simple users can use the simplified Context/Session management way.

This also needs to happen with the Ruby version as well. That suffers from the same dependencies.