CPIGroup / phpAmazonMWS

A library to connect to Amazon's MWS web services in an object oriented manner
Apache License 2.0
258 stars 228 forks source link

Pass MWS keys without config file #97

Open haffoudhi opened 8 years ago

haffoudhi commented 8 years ago

Is this possible ? I need to pass different MWS keys every time, and one config file is not the correct way. Is it possible to pass the keys directly without config files?

Peardian commented 8 years ago

I was under the impression that MWS Auth tokens were created once, at the same time as the Seller ID. Could you elaborate on your setup a bit? I need to know if this is an edge case or not before I decide on what to do about it.

haffoudhi commented 8 years ago

I m building an application for multiple sellers, each seller provides his own MWS auth token and seller ID. The app must do multiple reports at the same time for multiple sellers, using one config file is not an option here. The solution i m using is creating a different config file for each user and passing path to config file in every call to "phpAmazonMWS". I would prefer a more simple way, where i can introduce to api keys dynamically in every call, e,g new AmazonProductList($seller_id, $marketplace_id, $key_id, $secret_key..) not using a config file.

Peardian commented 8 years ago

That sounds like something worth supporting. I don't know how long it will be before I can work on it, but look into adding this feature when I can. Thank you for bringing it up.

ahmettok commented 8 years ago

I think it is a great feature to add, I also use the library for multiple users and I generate a new config file, every time I run the application, Queue the requests so that only 1 job is done.

morphers82 commented 8 years ago

There are tons of apps out there that access MWS on behalf of their users, this is definately not an edge case, in fact i think accessing mws for just a single seller is probably more of an edge case than accessing using a Developer Accounts Key/SecretKey plus the user's merchantId and a MWSAuthToken that has been issued by Amazon to be used by the developer account to access the MWS API on behalf of the merchant. Thanks!

stevechilds commented 8 years ago

I would echo this. I'm developing a integration using Symfony and its really not ideal to have to configure a php script within the vendors folder OR as I've done, create another PHP script in app/config and pass it into every constructor in the library when they're called.

The easiest solution would be to change it to accepting an array in the same parameter and doing a simple is_array() check, if it is, grab them, from there, if not use the existing code, then it doesn't introduce a BC break.

Edit: Actually is made worse by the fact the config param changes position with various constructor calls. I had planned to have a factory approach to generating them, this makes that harder as a now need to account for the fact that different constructors have them in different argument positions. :/

Peardian commented 8 years ago

My plan was to allow the first parameter, normally the store name, to be substituted for the array. After all, why would you need to give the store name if you're passing exactly the requirements you need? Unfortunately, I cannot work on it right now.

stevechilds commented 8 years ago

I've started to look into this, but oh hell! You've used the config file to set the store variable locally within each method it gets called in! :/ OMG, I can see why its a nightmare now. Sorry, but that's really, really awful design.

Ok, I've just searched and "include($this->config)" may only be in just under 30 places. What I would suggest (and I don't mind doing this, as long as the library does what we need it to do), is changing it so that the stores are read in and saved in the core class when its constructed, so that you don't need to include the config file each time you want to access them. You can pass them as an array in the store param as you suggest, these then get saved into the AmazonCore class.

Then wherever in the code include($this->config) is called, the method is changed to get the stores from the class property rather than the $store variable within the scope of the method.

The revised existing methods would still work with the current config file method as that file would be read in once in the AmazonCore constructor and saved into the object in the same way.

Thus accessing the data is the same irrespective of whether the config data is file based or array based.

Out of interest, why weren't the stores persisted into the Core object, as you do for some of the other settings?

Peardian commented 8 years ago

I won't defend my code on that point. After all, I wrote it 3-4 years ago as my first library.

I think that persisting the config data to the object would be a good idea, though I am not sure how exactly that could be done without changing the structure of the config file. After all, the config file contains several variables in addition to the store array, and all of that information needs to be passed to any created child objects. Those variables might need to be added to the store array anyway for the "config without file" idea to work. Perhaps that is why I designed it this way, so that only a single file path needs to be handed down.

Maybe there could be a configuration object? It could handle all of the loading/storing of config information and would simplify the passing of configuration to other classes. Just a thought that came to me while reading your message.

Regardless of how the store settings are saved, I think only the relevant store's array should be kept rather than all of them. I want to keep as little sensitive information stored in the object as necessary.

stevechilds commented 8 years ago

@Peardian I appreciate the code is old, I wrote some really bad code in the early days! We'll sort it though :)

I was thinking along the same lines and have created a configuration class/object which either takes an array or a file to keep backward compatibility. The core classes then refer to the config object rather than including the file all the time.

I've got it up and running with the simple test (getServerStatus) I had, now I just need to address the 221 test failures!

I've moved the logFile validation into the config class too, so I also solved the LogFile creation issue at the same time.

I'll add some new test cases too for the new functionality :)

I am trying to keep breaking changes to a minimum, so far there shouldn't be any! (fingers crossed)

stevechilds commented 8 years ago

Done :) All tests pass! I'll do a bit more testing, make sure I haven't missed anything and I'll commit the changes for you to review later today.

stevechilds commented 8 years ago

Ok, Pull request #103 added for your review @Peardian

Fabr9193 commented 7 years ago

Hello,

I cloned this github today and I was wondering if there is some documentation about this new way of passing MWS somewhere on the repo.

Thanks in advance !

stevechilds commented 7 years ago

There should be a demo for using it? I added it in my fork before submitting the pull request. This is my fork: https://github.com/schildsCC/phpAmazonMWS

Fabr9193 commented 7 years ago

@schildsCC Indeed there is I searched a bit more and I found your fork ;)

Thanks !

Fabr9193 commented 7 years ago

Is it possible to include your repository in packagist ? Because I'm having the minimum-stablity issue when I try to install this repo with composer

stevechilds commented 7 years ago

I'm a bit busy today and I've never done that, but I'll have a look at it this evening. I don't really want to create another fork that ultimately I won't maintain as I'm about to leave this job where I needed to use the library, at my new job I won't be using it and thus, can't really guarantee maintaining it.

What I did with my symfony project was to copy it into the src folder and used it from there - I don't know if you could do something similar as a temporary measure (admittedly its not ideal).

There's loads of abandoned forks around, I just don't want to add to the pile!

Regards

Steve.


From: Fabr9193 notifications@github.com Sent: 23 November 2016 10:10:42 To: CPIGroup/phpAmazonMWS Cc: Steve Childs; Mention Subject: Re: [CPIGroup/phpAmazonMWS] Pass MWS keys without config file (#97)

Is it possible to include your repository in packagist ? Because I'm having the minimum-stablity issue when I try to install this repo with composer

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/CPIGroup/phpAmazonMWS/issues/97#issuecomment-262475186, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AVaTGBdIUgFSm4gliicvA8I2Y09Bqfsaks5rBBEigaJpZM4JeU2P.

Fabr9193 commented 7 years ago

I understand 😄 I think i'll go for the temporary solution for now on.

Thanks

Fabr9193 commented 7 years ago

@schildsCC When I try your example (getAmazonFeedStatusB) in the examples Folder it says that I can't reach the value of secretKey which is in [stores[storeName[secretKey]]]

Here is my code : ` $amazonConfig = array( 'stores' => array('YourAmazonStore' => array( 'merchantId' => 'AAAAAAAAAAA', 'marketplaceId' => 'A13V1IB3VIYZZH', 'keyId' => 'AAAAAAAAAAAAAAAAAA', 'secretKey' => 'AAAAAAAAAAAAAAAAAAAAAAAAAA', 'serviceUrl' => '', 'MWSAuthToken' => 'amzn.**', ) ), 'AMAZON_SERVICE_URL' => 'https://mws-eu.amazonservices.com', // eu store 'logpath' => DIR . './amazon_mws.log', 'logfunction' => '', 'muteLog' => false );

    $anotherTest = new \AmazonMWSConfig();
    echo "Creation instance MWSConfig";
    $anotherTest->addStore('YourAmazonStore',$amazonConfig);
    dump($anotherTest->getStore('YourAmazonStore'));
    dump($this->syncFeed($anotherTest));
    exit();
}

public function syncFeed(\AmazonMWSConfig $configObject)

{
    $amz=new \AmazonFeedList($configObject->getConfigFor('YourAmazonStore'));
    $amz->setTimeLimits('- 24 hours'); //limit time frame for feeds to any updated since the given time
    $amz->setFeedStatuses(array("_SUBMITTED_", "_IN_PROGRESS_", "_DONE_")); //exclude cancelled feeds
    $amz->fetchFeedSubmissions(); //this is what actually sends the request
    return $amz->getFeedList();

}`
stevechilds commented 7 years ago

@Fabr9193 Your code is incorrect... look at the function getAmazonFeedStatusB() in the example..

You need to pass the array through to the AmazonMWSConfig constructor and remove the addStore call, i.e.

$anotherTest = new \AmazonMWSConfig($amazonConfig);
echo "Creation instance MWSConfig";
//  $anotherTest->addStore('YourAmazonStore',$amazonConfig);
// ...
dump($this->syncFeed($anotherTest));
Fabr9193 commented 7 years ago

Indeed it now works ! Yeah I was just desperate I might have changed some of my code and forgot to put it right.

Thank you !

buddyy93 commented 6 years ago

@stevechilds hi, i just tried your codes, but i'm getting this error.

A PHP Error was encountered

<p>Severity: Notice</p>
<p>Message:  Undefined index: code</p>
<p>Filename: classes/AmazonCore.php</p>
<p>Line Number: 605</p>
<p>Backtrace:</p>
<p style="margin-left:10px">
        File: E:\MyServer\spapp\vendor\cpigroup\php-amazon-mws\includes\classes\AmazonCore.php
    <br />
        Line: 605
    <br />
        Function: _error_handler
</p>

can you help? thank you

buddyy93 commented 6 years ago

sorry,there are my codes.

$amazonConfig = array( 'stores' => array('sales_planet' => array( 'merchantId' => 'HIDDEN', 'marketplaceId' => 'HIDDEN', 'keyId' => 'HIDDEN', 'secretKey' => 'HIDDEN', 'serviceUrl' => 'https://mws.amazonservices.com/', 'MWSAuthToken' => '', ) ), 'AMAZON_SERVICE_URL' => 'https://mws.amazonservices.com/', 'logpath' => DIR . './logs/amazon_mws.log', 'logfunction' => '', 'muteLog' => false ); $configObject = new AmazonMWSConfig($amazonConfig); $obj = new AmazonReportRequest($configObject->getConfigFor('sales_planet')); //store name matches the array key in the config file $obj->setStore('sales_planet'); $obj->setReportType('_GET_MERCHANT_LISTINGSDATA'); $obj->setTimeLimits('2018-08-01', '2018-08-02'); $obj->requestReport(); //this is what actually sends the request $requestreport = $obj->getResponse(); print_r($requestreport);