Skip to content

Add api key finder class and related configurations #386

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

badger-adam
Copy link

@badger-adam badger-adam commented Feb 3, 2025

This pull request introduces a new mechanism for resolving API keys in the shopify-app configuration. The changes include the addition of a new class and interface for finding the current API key, modifications to the existing utility function to support this new mechanism, and updates to the configuration file to allow for the use of a resolver class.

The driving force behind this PR is that the current method of a callback/closure will break the serialization of the configs so they can not be cached.

Key changes include:

New API Key Finder Mechanism:

  • New Class and Interface:
    • Added CurrentApiKeyFinder class to handle the resolution of API keys based on the shop domain. (src/ApiKeyFinder/CurrentApiKeyFinder.php)
    • Added CurrentApiKeyFinderInterface to define the contract for the API key finder. (src/Contracts/CurrentApiKeyFinderInterface.php)

Updates to Utility Function:

  • Enhanced API Key Retrieval:
    • Modified getShopifyConfig function to use the new config_api_class for resolving API keys if defined. (src/Util.php)

Configuration File Updates:

  • New Configuration Options:
    • Added comments and options in shopify-app.php to configure the config_api_class and explain its usage. (src/resources/config/shopify-app.php) [1] [2]

@badger-adam
Copy link
Author

The .env files are not loaded when the config is cached. So we have two options IMO. We can either (1) add every Shopify app's configs into the config/shopify-app.php file load them from the .env file (as I did in this PR) or (2) we can create a database table that contains them. Database table is more dynamic because you don't need to make a code change and deploy in order to add another Shopify app to the project. I think having the default behaviour of (1) is more ideal and if a specific project necessitates (2), they can implement their own CurrentApiKeyFinder class that loads the keys from a custom database table.

@Kyon147
Copy link
Owner

Kyon147 commented Mar 25, 2025

@badger-adam thanks for this PR.

Do you use the api callback function in your apps at the moment? I've not seen anyone use this feature and would love to know if you do and what you use it for.

@badger-adam
Copy link
Author

badger-adam commented Mar 25, 2025

@Kyon147 I work for an agency and we build a lot of Apps for custom functionality but we always try to "productize" the apps so that they are multi-tenant.

Shopify limits a Custom app to be installed on a single Shop or all Shops under a single organization. We set up multiple Shopify Apps that point to the same instance of our application and dynamically swap the API keys for each shop.

The existing solution breaks config caching because it's not serializable so this just aims to solve that problem while also making the solution flexible enough for database storing of keys if one desires.

@Kyon147
Copy link
Owner

Kyon147 commented Mar 25, 2025

Hey @badger-adam

Thanks for the reply, the context is really helpful.

I wonder if part of this PR, if you could add to the wiki how others can benefit from this by using the similar approach. I don't expect a full in-depth write up but a slimmed down version of how you use the app and how you can get it set up in a basic function will help tie this PR into a usable case for others too.

Would you be willing to do that? I can review this PR shortly so we can get it out in another release.

@Kyon147 Kyon147 added the PR: More Info needed Needs more clarity or context label Mar 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: More Info needed Needs more clarity or context
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants