Make bundle compatible with Symfony 4

[brianfreytag]

Fixes service definitions to make services explicitly public because they are private by default in Symfony 4.

Resolves #32 Closes #29

[brianfreytag]

I will check out these failures in 7.1. I did not receive them in 7.2 locally. I will also add checks for 7.2

[brianfreytag]

The failing tests from my initial PR were because of the dynamic service creation happening in AwsExtension. There is a test that pulls services from the container, and each and every one of those were breaking or failling. Symfony 4.0 reverses the default privacy of services. In order to get around this, I updated the service definitions to give them public visibility as they're being created.

[cjunge-work]

To whomever can do it: Can we please have this merged and a new release done as soon as possible? The versioning issue currently makes using this package with Symfony4 recipes impossible.

[cjunge-work]

@trandangtri wrote the recipe for Symfony4, maybe you can help merge this?

[trandangtri]

Hi @cjunge-work and @brianfreytag,

I'm not really sure that we truly need to mark all services of AWS as public as default or not. But IMHO, I prefer to use dependency injection instead. This increases the container's performance and easy to maintain, debugging, etc.

If dev has a specified reason to access any AWS service directly from the container, they could do it by themselves, via service alias e.g.

Just my 2 cents.

[cjunge-work]

@trandangtri I totally agree re. public as default. My issue is that the versioning constraints as it stands prevents using the bundle with Symfony 4. I can configure the services manually but would prefer to use the bundle with the recipe.

[brianfreytag]

@trandangtri @cjunge-work Taking services that were public as default and converting them to private as default will cause a lot of backward compatibility issues. Since this is a Symfony bundle, we can't really do that with a minor or patch version, so unless simply upgrading to Symfony 4 is going to be a major release version ~2.0, then I suggest we keep them public. Symfony bundles, imho, should follow the BC rules of the the parent project.

[brianfreytag]

@trandangtri Any further thoughts on this?

[cjunge-work]

If setting every service public is a concern, then for Symfony 4 could simply not expose the services as public. Kernel::VERSION would support version testing to enable/disable the public service. Having the SDK public is ok to me, but could be modified after it's configured.

[trandangtri]

CC @jeskew

[kstich]

We should keep this bundle in line with Symfony's idioms, which means leaving the change to private as default. Now, when Symfony has taken a major version with the change, seems like the right time to take a new major version of this bundle.

The original Travis job with failures can be found here.

[brianfreytag]

@kstich I'll make the changes, then. When it gets merged, we just need to make sure it gets tagged as 2.0.

[brianfreytag]

@kstich The only issue I see is (and the MAIN reason I set them as public) is that there is a test called all_web_services_in_sdk_manifest_should_be_accessible_as_container_services that checks to make sure that you can pull them from the container.

This test was added by @jeskew in the initial commit in 2015.

I would like to get his opinions.

[jeskew]

Hi @brianfreytag,

@kstich and I discussed this offline and were swayed by the argument that fetching a service by name from the container is not a symfony best practice. Bumping the major version and updating the test to reflect current symfony idioms and conventions would be preferable to strict backwards compatibility.

[brianfreytag]

@jeskew Thank you. I appreciate your feedback. I will undo my changes and remove that test. I'll have it up within the hour.

[brianfreytag]

@jeskew @kstich I've pushed up the updates. I modified one of the tests because I felt bad removing so many of them because they all checked that things got pulled correctly from the container, which, obviously, won't work anymore.

Just a reminder that this will need to be tagged at ~2.0 so that we don't break everybody who has been relying on the container.

[brianfreytag]

@jeskew I updated the tests. As for: sdk_config_should_be_passed_directly_to_the_constructor_and_resolved_by_the_sdk, I simply used the same idea you had for the other ones. I updated some regions for a couple of sdk configurations and tested against the services I injected into my new "TestService" class.

[brianfreytag]

@jeskew @kstich @trandangtri

Can we take a look at this again, please?

[brianfreytag]

@jeskew stupid auto-populated copyright notice removed.

[cjunge-work]

waits impatiently... :)

[kstich]

v2.0.0 has been released! Thanks!

[cryptiklemur]

Thank you!