Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve "Private App" API usage #151

Open
4 tasks
MikeParkin opened this issue Mar 8, 2022 · 44 comments
Open
4 tasks

Improve "Private App" API usage #151

MikeParkin opened this issue Mar 8, 2022 · 44 comments

Comments

@MikeParkin
Copy link

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:

  • Set Context::$IS_INITIALIZED (to prevent an exception)
  • Default Context::$HTTP_CLIENT_FACTORY = new HttpClientFactory();

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

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

Possible Improvements

  • Remove the need to call Context::initialize
  • Default the API version to 'unstable'
  • Default the initialization of $HTTP_CLIENT_FACTORY if null
  • Don't throw an exception during log if not initialized

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

@schmoove
Copy link

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

@MikeParkin
Copy link
Author

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
Copy link
Author

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 Shopify/shopify-api-ruby#911

@MikeParkin
Copy link
Author

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

#100
#104
#120
#176

@lukeholder
Copy link

lukeholder commented May 31, 2022

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
Copy link

lukeholder commented Jun 8, 2022

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
Copy link

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

@Zelfapp
Copy link

Zelfapp commented Jul 28, 2022

@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
Copy link

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.

@github-actions github-actions bot added the Stale label Sep 30, 2022
@lukeholder
Copy link

This should not be closed.

@github-actions github-actions bot removed the Stale label Oct 1, 2022
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

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.

@github-actions github-actions bot added the Stale label Dec 1, 2022
@MikeParkin
Copy link
Author

Still shouldn’t close this.

@github-actions github-actions bot removed the Stale label Dec 2, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot added the Stale label Jan 31, 2023
@lukeholder
Copy link

Still shouldn't close this.

@github-actions github-actions bot removed the Stale label Feb 1, 2023
@flogaribal
Copy link

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
Copy link

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
Copy link

elburro1887 commented Mar 29, 2023

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
Copy link

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.

@github-actions github-actions bot added the Stale label May 29, 2023
@lukeholder
Copy link

Still an issue with the packages PHP API

@github-actions github-actions bot removed the Stale label May 30, 2023
@ryanm-bw
Copy link

ryanm-bw commented Jun 2, 2023

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
Copy link

github-actions bot commented Oct 3, 2023

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.

@github-actions github-actions bot added the Stale label Oct 3, 2023
@elburro1887
Copy link

Issue is still not resolved.

Also: GitHub stale bot considered harmful

@github-actions github-actions bot removed the Stale label Oct 4, 2023
@ZacharyDuBois
Copy link

ZacharyDuBois commented Oct 16, 2023

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: Shopify/shopify-api-ruby#911 (comment)

@ZacharyDuBois
Copy link

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.

Copy link

github-actions bot commented Feb 4, 2024

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.

@github-actions github-actions bot added the Stale label Feb 4, 2024
@ZacharyDuBois
Copy link

Definitely not stale and stale bot is useless.

@github-actions github-actions bot removed the Stale label Feb 5, 2024
@lukeholder
Copy link

Not stale.

@benblub
Copy link

benblub commented Mar 28, 2024

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

Copy link

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.

@github-actions github-actions bot added the Stale label May 28, 2024
@MikeWillis
Copy link

Not stale

@github-actions github-actions bot removed the Stale label May 29, 2024
@fulldecent
Copy link

fulldecent commented Jun 13, 2024

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
Copy link

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

@fulldecent
Copy link

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

@alexwhb
Copy link

alexwhb commented Jul 12, 2024

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
Copy link
Contributor

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
Copy link

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.

@haoxi911
Copy link

I decided to use the raw HTTP requests instead of this SDK for now. I only need to read and update inventory with a private app. Will come back later when OAuth is needed to us. Thanks for everyone's comments here, it proves that I am not alone.

@MikeWillis
Copy link

@haoxi911 funny, I did the same thing a month or so ago. In hindsight I wish I never bothered with the SDK. It's been smooth sailing ever since I stopped using it.

@weinraum
Copy link

@haoxi911 thanks for your fresh comment. could you submit an example? After some days of completely useless trying, I'm done with messing around. Just need to access product data but can't get a connection to the shopify server.

cheers, thomas

@hectnandez
Copy link

Hi guys..., I'm just saying, thank god I reviewed this issue, I have been struggling with the documentation because it said too much but nothing at the same time.

So this is my implementation using Laravel:

<?php

namespace App\External\Shopify;

use Shopify\Clients\Rest;
use Shopify\Exception\MissingArgumentException;

abstract class BaseShopifyApi
{
    protected Rest $client;

    /**
     * @throws MissingArgumentException
     */
    public function __construct()
    {
        $this->init();
        $this->client = new Rest(
            domain: config('services.shopify.store_url'),
            accessToken: config('services.shopify.admin_token')
        );
    }

    private function init(): void
    {
        \Shopify\Context::initialize(
            apiKey: config('services.shopify.api_key'),
            apiSecretKey: config('services.shopify.api_secret'),
            scopes: config('services.shopify.scopes'),
            hostName: config('app.url'),
            sessionStorage: new \Shopify\Auth\FileSessionStorage(storage_path('framework/sessions')),
            isEmbeddedApp: false,
        );
    }
}
<?php

namespace App\External\Shopify;

use Illuminate\Http\Response;
use Illuminate\Support\Facades\Log;

class ProductsShopifyApi extends BaseShopifyApi
{
    public function list():? array
    {
        try {
            $response = $this->client->get('products');
            if($response->getStatusCode() !== Response::HTTP_OK){
                throw new \Exception($response->getDecodedBody());
            }
            return $response->getDecodedBody();
        } catch (\Throwable $exception) {
            Log::error($exception);
            return null;
        }
    }
}

be sure to use this help section https://help.shopify.com/en/manual/apps/app-types/custom-apps to generate all the credentials, scopes, etc that you need to connect with "your" store.

@lukeholder
Copy link

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

@paulomarg this is one of the things that needs to be fixed btw in addition to the other bugs.

@haoxi911
Copy link

@weinraum Here is an example. The tradeoff of using raw HTTP requests is probably composing GraphQL queries by ourselves. You could find the details in the Shopify GraphQL API reference.

$query = <<<'GRAPHQL'
query inventoryItems($first: Int!, $locationId: ID!, $after: String) {
    inventoryItems(first: $first, after: $after) {
        edges {
            node {
                id
                tracked
                sku
                inventoryLevel(locationId: $locationId) {
                    quantities(names: ["available", "on_hand"]) {
                        name
                        quantity                                        
                    }
                }                        
            }
        }
        pageInfo {
            hasNextPage
            endCursor
        }
    }
}
GRAPHQL;

$response = Http::acceptJson()
    ->withHeader('X-Shopify-Access-Token', xxxxxxxx)
    ->post(https://xxxxxxxx.myshopify.com/admin/api/2024-07/graphql.json, [
    'query' => $query,
    'variables' => [
        'first' => 250,
        'locationId' => $locationId, // PHP variable
        'after' => $cursor
    ]
]);

return $response;

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

No branches or pull requests