Kyon147 / laravel-shopify

A full-featured Laravel package for aiding in Shopify App development
MIT License
353 stars 102 forks source link

Simpler configuration #192

Closed bilogic closed 10 months ago

bilogic commented 1 year ago

I was debugging my webhooks and find that the current way of configuring via ENV to be quite cumbersome as most of the values are not considered sensitive enough to be in .env.

In my particular case, APP_UNINSTALLED had to be added and that info can only be found in the wiki. I mean, I can't think of any use case where APP_UNINSTALLED doesn't have to be defined, so it should come enabled by default. And we can do away with the wiki.

Here's my conclusion

  1. Define an array of configurations using the same structure as database.php's connections
  2. The configuration can be switched using SHOPIFY_CONFIGURATION in .env
  3. For sensitive info such as SHOPIFY_API_SECRET, read them from the .env using the same keys

Example

return [
    'default' => env('SHOPIFY_CONFIGURATION', 'production'),
    'configurations' => [
        'production' => [
            'id' => ENV('SHOPIFY_API_KEY'),
            'secret' => ENV('SHOPIFY_API_SECRET'),
            'key' => ENV('SHOPIFY_API_ID'),
            'webhooks' => [
                'APP_UNINSTALLED' => ENV('APP_URL').'/webhook/app-uninstall',
            ],
        ],
        'development' => [
            'id' => ENV('SHOPIFY_API_KEY'),
            'secret' => ENV('SHOPIFY_API_SECRET'),
            'key' => ENV('SHOPIFY_API_ID'),
            'webhooks' => [
                'APP_UNINSTALLED' => ENV('APP_URL').'/webhook/app-uninstall',
            ],
        ],
    ],
];
Kyon147 commented 1 year ago

@bilogic other than the uninstall webhook needing to be added by default - I'm not sure the need for the configurations array, can you give an example of a use case?

bilogic commented 1 year ago

@Kyon147

I gave your question more thought. database.php used it for different variants of SQL. Here, there isn't any variant, only different environments, so configurations is probably not needed.

But I think removing webhooks from .env and perhaps adding all known webhooks but have them commented out will still help :)

Thank you.

bilogic commented 2 months ago

I gave this even more thought and can clearly identify the problem now, specifically webhooks.address, what we want to configure per environment is really just the domain, not the path segment e.g. /webhook/order-create. There is no reason for path segments to be different for each environment.

There is really no need for an app developer to be typing path segments (which they are most likely unfamiliar with) into the .env or config file.

Basically, the topic and path segments should be fixed by this package. The only thing modifiable by the app developer is just the prefix of these URLs and this will allow one to avoid URL clashes.

What we can have in the config file is a list of topics => handled by which class, and each can be commented out if not being handled.