Cleanup module

[bakura10]

Hi,

This PR slightly clean up the module:

• I couldn't run the tests because it could not load the vendor/autoload file. I've made it a little more robust, based on what we did on DoctrineModule.
• This is a little BC but I think this module should not be under namespace Aws but AwsModule (as it is for DoctrineModule, DoctrineORMModule, for the only reason we may have clashes).
• I removed the closure in Module.php and replaced it by a factory instead, it's a bit more flexible as people can eventually override it so they can introduce their own things when creating an AWS client (well, closure can be overloaded too but I find it strange to introduce object creation in Module.php, it's clearer in a separate file).

Thanks ;-).

[bakura10]

(If this PR is merged, please could you tag it so I'd like to use it for SlmQueueSqs ;-)).

[jeremeamia]

Thanks! We will review these changes and see how we want to proceed. I'd also be interested in what @EvanDotPro thinks of the changes since he helped me with the module originally. Before I can accept your PR though, I will need you to sign a CLA. Please send me an email using the email address listed on my profile. Thanks.

[jeremeamia]

Thanks, I have the CLA now.

Is changing the namespace really necessary? I'm not sure I understand why that BC would help.

[bakura10]

Hi,

No, changing the namespace is not really necessary. I can removed it. I just thought that, because I introduced Aws\Factory, you may have one in the official Sdk with potential name clashes.

For instance in laravel you did Aws\Laravel\something. In ZF 2 modules the convention was to use SomethingModule (like DoctrineModule, DoctrineORMModule....).

If this is really a problem I can change it back to Aws.

Envoyé de mon iPhone

Le 21 mars 2013 à 00:12, Jeremy Lindblom notifications@github.com a écrit :

Thanks, I have the CLA now.

Is changing the namespace really necessary? I'm not sure I understand why that BC would help.

— Reply to this email directly or view it on GitHub.

[jeremeamia]

Let's keep the namespace as Aws, since that is the correct vendor name, IMO, and does not require breaking changes. The factory could live in Aws\Zf2\AwsFactory. None of your other changes would be considered BC, right?

I like the changes. I am still waiting for two people to review and approve this PR, so I'll get back to you when that is complete.

Thanks.

[bakura10]

Hi, I renamed it to Aws to remove BC.

I decided to keep the simpler Aws\Factory\AwsFactory, it's less ugly than Aws\Zf2\Factory\AwsFactory ;-). In any case, if Aws release a new component called Factory with a class called AwsFactory inside (which I doubt), we can still change the factory name without causing BC (it's just a matter of changing the config file).

So I think it's ok now ;-).

[bakura10]

Any news about this ? :)

@ocramius ping, could you just review quickly the code even if you don't know anything about Aws ? :)

[jeremeamia]

Still waiting on a review from one more person that I've contacted. Should be able to take care of this within the next couple of days. Sorry for the wait.

[bakura10]

Thanks for the feedback Marco !

[EvanDotPro]

@bakura10 While you're at it, a phpunit.xml would be handy, even if it just points to bootstrap.php.

[bakura10]

There is one already EvanDotPro :).

[EvanDotPro]

Haha wow, sorry, I'm operating on about 40 minutes of sleep this morning.

[bakura10]

@EvanDotPro I have a question for you regarding Composer: https://github.com/aws/aws-sdk-php-zf2/pull/2/files#L2R14

As you can see, now both Aws SDK and this module have the same value here. Is composer smart enough to specify an array in autoload_namespaces.php ?

[Ocramius]

@bakura10 you should have a different namespace then.

[EvanDotPro]

Might be useful to add "zendframework/zend-modulemanager": "2.*" and "zendframework/zend-version": "2.*" to composer.json.

[bakura10]

I'm just asking. That's why I wanted to rename this "AwsModule" instead of Aws, but @jeremeamia does not agree with that because of the small BC it implies.

[bakura10]

Ha good catch for zend-version Evan ! ModuleManager is really needed to run the tests instead right ? (so as require-dev ?)

[EvanDotPro]

@bakura10 Looking at the composer source, it should handle multiple libraries registering two paths for on namespace prefix just fine. When the add() method is called, it will simply merge the new path(s) added with the existing path(s) registered for that prefix/namespace.

[EvanDotPro]

@bakura10 Yeah -- assuming the module is being loaded by anything else besides the unit tests, then it's pretty safe to assume the ModuleManager has been included somehow already... Otherwise how would it be getting loaded in the first place? So yeah, require-dev might be appropriate, but there's also no reason not to just require it normally either.

[EvanDotPro]

I think I agree with @jeremeamia. I don't see a reason to change the namespace from Aws as long as it doesn't cause autoloading problems, which it shouldn't as far as I can tell.

[bakura10]

I have updated everything. Thanks everyone for the feedbacks.

[jeremeamia]

These changes are looking good. Thanks to the 3 of you. However, I'm uneasy about a couple things still.

It looks like you've changed the way the config retrieval logic works. It now throws an exception when no config is provided. See https://github.com/bakura10/aws-sdk-php-zf2/blob/cleanup/src/Aws/Factory/AwsFactory.php#L39-L43. I don't believe this is a good idea since you can use the SDK without providing configuration up front. For example, if you were just going to use the S3 client (which does not require a region) and your code was relying on environment or EC2 Instance Metadata credentials, then you would not need to explicitly provide any configuration. Throwing an exception would prevent this and similar use cases from being possible via the module.

@Ocramius and @bakura10: What was wrong with the way the config retrieval was being handled before? @EvanDotPro In your opinion, what is the "correct" way to get the config data if it exists?

I'm weary of the factory's FQCN being Aws\Factory\AwsFactory. I think I'd prefer Aws\Zf2\AwsFactory. This would better match the pattern I've followed with other framework modules and clearly delineate that it is for ZF2. See https://github.com/aws/aws-sdk-php-laravel/blob/master/src/Aws/Laravel/AwsServiceProvider.php and https://github.com/aws/aws-sdk-php-silex/blob/master/src/Aws/Silex/AwsServiceProvider.php.

[bakura10]

Yes, I've added an exception. Basically, it should never throw this exception because once the module is activated, the default config already contains the "aws" key, so this is really "exceptional" and should never happen (and it's better to throw an exception if no config is set that silently fails).

Then, if you provide "aws" key without credentials, it behaves as before (throw InvalidCredential exception).

Concerning the namespace, we could do that. To be honest I'm not a big fan of it because this is not how we usually deal it within Zend Framework (factories are either under MyModule/Factory or MyModule/Service namespaces). I think each module should first be written according the framework rules. And Aws\Zf2\Factory\AwsFactory looks complicated.

Ideally, a rename to AwsModule would be the best of both worlds as I said previously (no clashes ever possible and follow ZF2 conventions).

[jeremeamia]

Yes, I've added an exception. Basically, it should never throw this exception because once the module is activated, the default config already contains the "aws" key, so this is really "exceptional" and should never happen (and it's better to throw an exception if no config is set that silently fails).

By "default config", do you mean the config the user must create by copying aws.local.php.dist? If so, then this would be a backwards incompatible change.

[bakura10]

What will happen:

1. If no "aws" key is here in config => throw RuntimeException saying that "No config was set". This is no BC because previously it was asked to manually add "aws" key into their config, now we ask them to copy-paste aws.local.php.dist, and modify it to their needs.
2. If "aws" key is here but no credentials is set => AWS will throw InvalidCredential exception.

[EvanDotPro]

@bakura10 Actually, @jeremeamia is right about the config -- we had it set up so the config was optional, so throwing this exception is, indeed a BC break.

While, yes, the documentation asked them to copy the config over, this was actually optional on purpose. Check out the source code to the actually AWS PHP library here. It has a default that it falls back to if no config is passed to the factory. I think at that level, it's the responsibility of the library to throw an exception if there's a config issue. Us throwing the exception here in the module is too soon.

[bakura10]

Haaa you're right, I didn't know that the AWS SDK fallback to Resources/sdk1-config.php. I'm going to change that.

[bakura10]

Done. Is it ok for everyone ?

[EvanDotPro]

Regarding the Aws\Factory\AwsFactory vs Aws\Zf2\AwsFactory issue -- Personally, I think consistency throughout the code that AWS releases should take priority over following ZF2 conventions for things like naming, etc. For example, we have actually stated that the "Module" suffix is not our naming convention -- however I was in favor of Doctrine naming their module DoctrineModule to be consistent with their other projects such as the SF2 DoctrineBundle, etc. Consistency across their projects should be a priority before conforming to the conventions of an external project. It's just a matter of opinion, though.

[bakura10]

In any case, it would be either Aws\Factory\AwsFactory or Aws\Zf2\Factory\AwsFactory, not Aws\Zf2\AwsFactory. The more I think about it, the more the name clash seems very unlikely. I really doubt AWS will ever release a component called "Factory". And even if they do, it's super simple to change it here without any BC (just change the mapping in module.config.php so that 'Aws' have another factory).

[EvanDotPro]

@bakura10 Looks good.

As I mentioned on IRC, I'd personally still suggest dropping the config/ directory, and rather just return the config array with the factory registration from the getConfig() method, and leave the "dist" config in the readme. Again, this is more of a matter of opinion not so much a technical or even best practice argument, really -- so ultimately if @jeremeamia is good with it, I can get over my minimalist OCD and let it slide. :)

[bakura10]

I'm so accustomed at going to config and copy-paste the .dist file, without even reading the README. I've applied this schema to nearly all the modules I've written. I'm going to stop here because I like it more like it is now =). If @jeremeamia absolutely needs that I change something I'll do it but I'm happy with that.

(same applies to config, I'm always watching the module.config.php file now, I tended to remove all the config stuff in Module.php now to keep it as clean as possible).

[EvanDotPro]

Yeah -- I was the one originally pushing for that convention way back in the early, early betas, so I'm definitely not against the practice. I just tend to try to keep my modules minimal where possible, while others prefer consistency over minimalism.

The point you make about people "expecting" or being familiar with the concept of .dist configs and config directory is definitely valid, and is enough for me to be fine with leaving it as-is. I like to live in my dream world where everyone reads the README before just expecting stuff to work, but alas, I know it's only a dream.

[jeremeamia]

Thanks everyone for your work on this. Unfortunately, I just can't accept this pull request...

April Fools! :wink:

Seriously though, thanks a lot. I like where this PR is at now, so I'm going to go ahead and merge it in. I'll tag soon as well.

[jeremeamia]

I did a little cleanup as well and refactored the tests. I also tagged a new version. Thanks again everyone.

[bakura10]

Ouch you got me for your april fools, I was like "WHAT AGAIN ?" :D.

Thanks for merging and the additioonal tests, look good !