Symfony 4 compatibility

[vasike]

Checked within Symfony 4 and Symfony 3.3 little app and it seems it works.

But more testing is required.

[toooni]

@vasike What do you think about setting the google_tag_manager service to public? Since the example in the readme is

/** @var GoogleTagManagerInterface $manager */
$manager = $this->get('google_tag_manager');
$manager->setData('example', 'value');

Without setting the service to public, we can't access it from the container anymore in sf4.

[vasike]

@toooni Hi

did you tried to update the usage as specified in symfony documentation https://github.com/symfony/symfony/blob/master/UPGRADE-4.0.md#dependencyinjection i mean to add the service as argument?

[toooni]

@vasike This sure works - but I wasn't talking about a specific problem. I just asked what the default behaviour should be. The example in the docs show the usage through the container (a lot of users are using controllers without service injection).

[vasike]

@toooni I can't say how it should be

Update the documentation to guide how to override the service and make it public for sf4 if needed in the container

or

Make it public here

maybe someone else will help us here.

Thanks

[stefandoorn]

I'm not sure if this works, but I'm in general favor of making it private and showing developers how to use dependency injection to access the service. In case anyone needs it public, maybe we can hint to this: https://symfony.com/doc/current/service_container/alias_private.html. The example describes going from public to private, but I assume it can also go private to public..

[toooni]

This might be ok and does work. The example does indeed show how to make a private service public. The service with the alias property is the one created from the one in alias.

[vasike]

@stefandoorn i also thought about this

services:
    google_tag_manager_public:
        alias: google_tag_manager
        public: true

and usage

      /** @var GoogleTagManagerInterface $manager */
      $manager = $this->get('google_tag_manager_public');
      $manager->setData('example', 'value');

[stefandoorn]

Alright. Only issue is then that changing this might incur BC. But, we could make a public alias with the old name, and introduce another service that's private and use this one in the README. @xyNNN and I were talking a while ago about making some other improvements which also hold BC, so that might be something for a new major then to swap out again. But yeah, takes also more effort instead of just providing S4 support :)

[stefandoorn]

Haha, great @vasike, exactly that :-)

[vasike]

Updated the README file

@stefandoorn what do you think? is there something else needed to be done? Should we "autowire" the service by default?

[xyNNN]

Thank you guys! I was in holiday, so I was a bit far away from my keyboard! I can agree with (@toooni) the argument that many developers not using controller as a service in symfony. Due to this fact we have to make it public, so anybody can access it directly through the container.

We should define the default service as public to not introduce a breaking change (The public/private visibility is available also in Symfony 3.x).

@vasike What do you want to autowire in the Service? It has no dependencies except some parameters which are currently defined through the factory.

[vasike]

@xyNNN , @stefandoorn Updates:

• I removed previous commit on README file - as it is not needed anymore.
• I made default service as public.

Are ok now?

p.s. Autowire for Controllers (just a thought/question)

[stefandoorn]

Looks good to me. The builds on PHP 5.3 and PHP 5.4 fail (not sure if it's related), but maybe it's better to stop supporting them anyway for new releases.

[toooni]

@vasike I don‘t think setting all services to public is a good idea. Why not just set the google_tag_manager to public? The factory or listeners shouldn‘t be public.

[stefandoorn]

Ah I missed that. Yes I agree, only the one to be used in the controller should be public.

[xyNNN]

I don't think that so many people uses PHP 5.3 or 5.4 but perhaps it makes sense that we do with this pull request the major release upgrade? What do you think @stefandoorn?

[xyNNN]

@vasike Please can you adjust the travis configuration and remove the old PHP versions 5.3 and 5.4 - Afterwards I will merge this and create a new major release.

[stefandoorn]

@xyNNN Don't forget the default public setting now, should be adjusted. Also I think we should aim on PHP >= 7.1 in a new major, going with strict type hint and return types. This one can be a minor maybe to support Symfony 4 and then move on to the major afterwards directly with dropping PHP support? Then we keep PHP 5.3 in here also. Idea?

[xyNNN]

Okay, yes ... you're right ... I think this is the better way to do this. Then we should try to solve the issues which the current build, any ideas around here?

[xyNNN]

It's strange because the definition for the default values are handled like it is a service definition.

[stefandoorn]

Isn't for 5.3 + 5.4 an older version of Symfony installed which doesn't understand _defaults yet?

[xyNNN]

Yes. Symfony 3.3 (which support autowiring, _defaults, etc) is only available for PHP greater than version 5.5.9 - so we have dropped the support for PHP 5.3 and 5.4 with introducing the _defaults in the service configuration. We should move it to each service definition and set it to public manually (see http://symfony.com/doc/3.3/reference/requirements.html)

[stefandoorn]

Yes. I think as mentioned above only one service needs to be public, the rest can be set to private then. Should be fairly ok to do it that way, we don't have hundreds of services :-)

[stefandoorn]

Should all be public to keep up how it used to be or do we only go with google_tag_manager being public as that's what was intended (README)? I've pushed the latter now to the branch of @vasike.

[xyNNN]

I think we should only set the google_tag_manager to be public because the right way is to use dependency injection and not getting the servies via the service container directly. Also non-used private services are not integrated into the cached service container, so we should not spam the service container with public services ;)