WIP

[MannikJ]

Hi,

this PR addresses some issues discussed in #2 and #3.

I decided against caching the classes and against putting the class finding logic in the config as well. I think having the service provider implement the DeferrableProvider interface as is, is already enough to reduce the performance impact in production environments. I had previously overlooked this.
I could verify that when the DeferrableProvider interface is added, the ServiceProvider is neither created nor registered or booted on normal HTTP requests. Also a robust caching mechanism which ensures that the classes are up to date at all times is not that simple to implement.

I would vote for listing all the API classes explicitly as well. The auto-discovery "feature" does not provide any benefit for the user, but has a high risk of doing harm. It only saves the package maintainer a few minutes ;)

I also removed the region check to be able to use EU_SANDBOX during tests. I still don't really see why this check is needed except because the region db column for the multiseller setup is defined like this:

$table->enum('region', SellingPartnerApi::REGIONS);

If we changed this, we could probably remove the REGIONS constant in SellingPartnerApi (public const REGIONS = ['NA', 'EU', 'FE'];) completely.

[jlevers]

The auto-discovery "feature" does not provide any benefit for the user, but has a high risk of doing harm.

Out of curiosity, what do you see as the risks of doing harm? I don't mind updating the list of classes occasionally, but I can basically guarantee I'm going to forgot to add new ones sometimes.

Re: the region value check, I'm fine removing it, but I'd like to keep the enum column for the multi-seller setup. If someone using the library wants to modify the migration when they publish it, they're welcome to, but that constraint prevents accidentally adding bad endpoints.

I made some minor comments – if you wouldn't mind addressing those and running composer lint, we should be good to go.

[MannikJ]

The auto-discovery "feature" does not provide any benefit for the user, but has a high risk of doing harm.

Out of curiosity, what do you see as the risks of doing harm? I don't mind updating the list of classes occasionally, but I can basically guarantee I'm going to forgot to add new ones sometimes.

Well harm is maybe a bit too much. What I mean is that what the ClassFinder library is actually not as easy as it looks. It might involve filesystem operations which is potentially very slow - as we noticed. So without a solid caching mechanism it will just be too slow for many usecases. Also a service provider is one of the worst places for slow code.
A caching mechanism should probably be implemented very similar to Laravel's event discovery. It's also not impossible to do, but it basically just moves the responsibility to pay attention for new classes from the maintainers of this package to its users.

Re: the region value check, I'm fine removing it, but I'd like to keep the enum column for the multi-seller setup. If someone using the library wants to modify the migration when they publish it, they're welcome to, but that constraint prevents accidentally adding bad endpoints.

Not yet deleted and will leave as is.

I made some minor comments – if you wouldn't mind addressing those and running composer lint, we should be good to go.

Yes I'll see that I can make the discussed changes within the next days.

[jlevers]

I just made the SP API classes listed explicitly. Once those revisions are ready and the conflicts are merged from that update, should be good to go :)

[jlevers]

Couple tiny comments, once those are addressed this'll be ready to merge :)

[jlevers]

Awesome, looking good. Thanks!