Closed gws closed 11 years ago
Cool. This looks good. To approve this pull request I nee you to confirm that you are fine with submitting this code under the Apache 2.0 license and have you sign a CLA. Please send an email to the email address located on my GitHub profile, and I'll send you the CLA. Thanks.
Hi, is there any more news about this pull request ? I would be very interested in using that save handler. Thanks
Yes. Thanks for the bump. I have received the CLA. I'll will merge this in this week.
Nice stuff! However, you should really write factory to create the save handler :).
@bakura10 Is this something that would be required for this pull request or can it be added afterwards?
It should be better to have it before tagging a new version. You can merge this and I can add it afterward if you want.
Only if you don't make me sign another CLA :d.
@bakura10 Ha ha ha! I probably/hopefully won't need it for a smaller contribution like that. I'm going to follow up with some people at work and see if maybe your CLAs (or possibly just one more) can apply to all future contributions for you. I totally appreciate your contributions and hope that the SDK and AWS are helping you with your work.
Of course, I use it in nearly all my projects ;-) (although I've never used DynamoDb).
Stay me tune (of course if @gwis could add the factory that would be even nicer)
@bakura10 I'd be happy to add the factory, as I mentioned in the pull request. Do you or @jeremeamia have a configuration key naming preference? As far as I can tell there is no precedent set here or in ZF2 (for other save handlers), which was my only concern.
Yes, follow the existing keys (in the module.config.php class). The key should be the FQCN of the class.
Envoyé de mon iPhone
Le 31 juil. 2013 à 21:13, Gordon notifications@github.com a écrit :
@bakura10 I'd be happy to add the factory, as I mentioned in the pull request. Do you or @jeremeamia have a configuration key naming preference? As far as I can tell there is no precedent set here or in ZF2 (for other save handlers), which was my only concern.
— Reply to this email directly or view it on GitHub.
Sure, for setting up the factory I will follow that convention. However, I'm talking about configuring the actual save handler (including, but not limited to, configuration keys that would ultimately be passed to Aws\Dynamodb\Session\SessionHandlerConfig
by the factory). An analogous example from code you will be familiar with would be the zfr_mailchimp
key, which is not the FQCN of the class: https://github.com/zf-fr/zfr-mailchimp-module/blob/master/config/zfr_mailchimp.local.php.dist
Haa I see. Of course, options should be underscore_separated. I was just talking about the factory key.
I thing the configuration should be handled by the Aws library, not the ZF2 module.
You should just fetch the session handler in the factory I think.
Envoyé de mon iPhone
Le 31 juil. 2013 à 21:38, Gordon notifications@github.com a écrit :
Sure, for setting up the factory I will follow that convention. However, I'm talking about configuring the actual save handler (including, but not limited to, configuration keys that would ultimately be passed to Aws\Dynamodb\Session\SessionHandlerConfig by the factory). An analogous example from code you will be familiar with would be the zfr_mailchimp key, which is not the FQCN of the class: https://github.com/zf-fr/zfr-mailchimp-module/blob/master/config/zfr_mailchimp.local.php.dist
— Reply to this email directly or view it on GitHub.
Ok, so I checked the code of the DynamoDB, so as the official module does not provide any thing, just add a few keys to the official module:
'aws' => array(
'session_config' => array(
// 'table_name' => '',
// other keys from here: https://github.com/aws/aws-sdk-php/blob/master/src/Aws/DynamoDb/Session/SessionHandlerConfig.php#L51
)
)
Then create a ZF2 factory for creating a session handler: https://github.com/aws/aws-sdk-php/blob/master/src/Aws/DynamoDb/Session/SessionHandler.php
(so a factory that map Aws\DynamoDb\Session\SessionHandler to Aws\Factory\DynamoDbSessionHanderFactory)
Then another factory that would map 'Aws\Session\SaveHandler\DynamoDb' to Aws\Factory\DynamoDbSaveHandlerFactory.
Makes sense to you ? :)
The SDK's Session Handler class has a factory method, so I think that you will only need to write a factory class for the module's SaveHandler.
Ha yeah I didn't see the factory. However from where does the factory read the config ? I didn't see any config keys.
Envoyé de mon iPhone
Le 31 juil. 2013 à 22:26, Jeremy Lindblom notifications@github.com a écrit :
The SDK's Session Handler class has a factory method, so I think that you will only need to write a factory class for the module's SaveHandler.
— Reply to this email directly or view it on GitHub.
Indeed. In order to allow users to configure the session handler, though, we need a place in the aws
configuration tree to place those keys, right? This would become part of the array I would pass to the factory method you point out, along with the DynamoDB client which we know how to fetch from the service manager.
e.g.
<?php
// somebody's local.config.php
return [
'aws' => [
'dynamodb' => [
// one possibility for naming
'session_handler' => [ /* SessionHandlerConfig keys, etc ... */ ]
]
]
];
Perfect naming convention ;-). But I think this may be moved to the PHP SDK (the factory reading from the aws config if the config array given to the factory is empty). This way even users not using ZF2 could benefit from it.
What do you think Jeremy ? The only drawback is that we must wait for a new tag of the PHP SDK and set a higher required version.
Envoyé de mon iPhone
Le 31 juil. 2013 à 22:32, Gordon notifications@github.com a écrit :
Indeed. In order to allow users to configure the session handler, though, we need a place in the aws configuration tree to place those keys, right? This would become part of the array I would pass to the factory method you point out, along with the DynamoDB client which we know how to fetch from the service manager.
e.g.
<?php
// somebody's local.config.php
return [ 'aws' => [ 'dynamodb' => [ // one possibility for naming 'session_handler' => [ /* SessionHandlerConfig keys, etc ... */ ] ] ] ]; — Reply to this email directly or view it on GitHub.
Ha yeah I didn't see the factory. However from where does the factory read the config ? I didn't see any config keys.
In the SessionHandlerConfig
class.
In order to allow users to configure the session handler, though, we need a place in the aws configuration tree to place those keys, right? This would become part of the array I would pass to the factory method you point out, along with the DynamoDB client which we know how to fetch from the service manager.
That array structure would makes sense, but wouldn't that get passed directly into the SDK? See the AwsFactory
lines 39-41. The SDK only cares about client configuration settings. I'm not exactly sure how those config values would make it to the SessionHandler/SaveHandler.
But I think this may be moved to the PHP SDK (the factory reading from the aws config if the config array given to the factory is empty). This way even users not using ZF2 could benefit from it. What do you think Jeremy? The only drawback is that we must wait for a new tag of the PHP SDK and set a higher required version.
No. Our config and service builder is only for client objects. We do not plan to support a full on config/DI system within the SDK. That's what frameworks like ZF2 are for.
@jeremeamia Thanks, knowing the demarcation between the clients and all other factories is useful. Ultimately I'm only trying to avoid stomping on a namespace you and other AWS PHP SDK maintainers might want to add to in the future. This configuration can live anywhere :)
I've just pushed another commit which adds a factory as discussed.
I've added a top-level key called aws_zf2
(a name which I'm totally open to changing), which will contain configuration for this ZF2 wrapper module that, for reasons you've already stated, we want to leave out of the PHP SDK proper, and is where configuration for this save handler and future configuration options for classes in this module would live, following ZF2 conventions.
Awesome! Nice work! @bakura10 Look good to you? @fdewinne How about you?
I totally agree with @bakura10 about the config file. I think you could merge when these changes are done. Thanks a lot for your work guys :)
@bakura10 Thanks for taking the time to review that. If everything looks :thumbsup:, let's land this thing.
So, are we ready to merge then?
I've done a few more feedbacks @jeremeamia , once it's done you can go on :)
Looks good! I'll merge this in after my lunch. Thanks @gwis for your time and code. Thanks to @bakura10 and @fdewinne for reviewing and supporting.
Now available in 1.1.0. Yay!
The AWS PHP SDK already has a great session save handler that uses a DynamoDB backend.
All this does is implement the ZF2 session save handler interface so that it can be used like any other save handler within ZF2.
If this would be a desired addition, I think creating a factory would be a good next step for ease of integration, but I'm not sure where the session handler configuration ought to live.
Thanks for your consideration!