Symfony bootstrap cleanup, the new 'kernel.reset' tag should do the job

[acasademont]

All the custom hijacking shouldn't be needed anymore afther SF 3.4 added the kernel.reset tag. Not so sure about the logger and swiftmailer hijacks

[acasademont]

Tests are failing because the Kernel mock explicitly throws an exception if bundles have not been initialized before the first request. Symfony boots the Kernel on the first request, so I'm not sure if those tests are still relevant...ping @marcj

[acasademont]

I believe that hacky bundle initialization was done in order to do some stuff with the session service a while ago, but that hack is no longer in place

[marcj]

The tag is only used when we'd call kernel->reset, which we don't. The thing is, when you reset the kernel and throw with it all service, basically the whole performance gain is gone as well.

[dnna]

@marcj actually this tag is used in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Kernel.php#L110, it is not related with kernel->reset.

Personally I think this PR makes sense with Symfony > 3, but is there a way to verify that the profiler is still working the same with the removed lines?

[marcj]

Oh, you're right, sorry about that. :)

Is services_resetter resetting the container or does it send a reset event to subscribers? If so, Symfony should've already covered a couple stuff we tried to fix via hacks.

but is there a way to verify that the profiler is still working the same with the removed lines? I guess we can only test that currently manually.

[dnna]

It doesn't reset the container, it just iterates the services tagged with kernel.reset and calls the reset method on them.

I think it covers most of stuff we fixed through hacks. In fact with, Symfony 4, most of the hack code doesn't even get executed because most services are no longer public :)

[acasademont]

Yes exactly, this is precisely the point of this commit, to remove some of the hacks php-pm had to do before now that symfony (since 3.4) has this kernel.reset tag.

Symfony 2 has been eol-ed already and it only gets security fixes. I believe this could go into the next release.

On Wed, 28 Nov 2018 at 15:44, Dimosthenis Nikoudis notifications@github.com wrote:

It doesn't reset the container, it just iterates the services tagged with kernel.reset and calls the reset method on them.

I think it covers most of stuff we fixed through hacks. In fact with, Symfony 4, most of the hack code doesn't even get executed because most services are no longer public :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/php-pm/php-pm-httpkernel/pull/144#issuecomment-442471180, or mute the thread https://github.com/notifications/unsubscribe-auth/AAyWvYhwrHoy_kXKwGHUZjPa-hw33Rppks5uzqFUgaJpZM4Y3aka .

[andig]

@marcj would like to leave this with you ;)

[marcj]

All up to you @andig, I haven't developed PHP for quite some time. :P But that reset tag sounds very good and goes directly in PHP-PM's direction. Love it :) We should test however if that stuff we covered via hacks still works after this PR 💃

[acasademont]

All the removed hacks have been tested with SF 4 only. I'd still like to know what you want to do with the tests.

I believe @dnna was working on a patch to make it work with symfony/monolog-bundle#279 so after that another of the hacks could be removed

[acasademont]

Actually @dnna PR doesn't seem related sorry! Seems like the getDebugLogger hack could also be removed, as that is triggered by the LoggerDataCollector clear which is triggered by the kernel.reset tag on the Profiler

[andig]

Regarding marc's comment:

We should test however if that stuff we covered via hacks still works after this PR 💃

Is there any way we can unit-test this before/after? For all affected services? As admitted before- I personally have no idea about Symfony :(

[acasademont]

I'll take a look at it. It shouldn't be too difficult to make those tests.

[andig]

ping @acasademont is this still something you're interested in?

[acasademont]

Yes, but I totally forgot about it :p I'll try to make them work during this weekend :)

[acasademont]

First step to bulding relevant tests is at #152

[andig]

@acasademont I understand we want to merge this after #152 to verify it still tests, right? Can we merge this one as part of a bugfix release? Please note that it also has conflict currently.

Btw., you can also remove the PHPPM\Symfony\StrongerNativeSessionStorage, its no longer utilized after this PR.

[acasademont]

@andig yes, but more tests are needed for that, #152 just lays the foundation for this future tests. Probably I should add them on the other PR and make the refactor in this one.

[acasademont]

And yes, this could be a minor/patch release, we're doing nothing fancy, just removing stuff

[andig]

ping @acasademont is this something you want to get into upcoming 2.0?

[acasademont]

Yes, I just need to make some time for it :(((

[andig]

Is this something we could get into a 2.1 release really soonish?

[acasademont]

@andig I cleand up a bit the tests and rebased with master. Some doubts though:

• Until when do we keep those checks. For example, the webpack bundle now already implements the ResetInterface but as I don't use it, I'm not sure when should we remove that check. ping @fionera
• Also, going forward, we shouldn't add all these in the core Symfony bootstrap, there are potentially an infinite number of libraries that need some kind of reset post-request, people should be able to extend and create their own bootstrap and reset their stuff there. On the core Symfony we should only deal with core Symfony stuff.

[andig]

Until when do we keep those checks.

Totally up to you. I really have no idea about the Symfony part :/

Also, going forward, we shouldn't add all these in the core Symfony bootstrap

Agreed, but that would need a new mechanism and is outside of this PR's scope.

So... merge for time being?

[acasademont]

Yes, I just removed the webpack checks too.

Travis seems to be failing due to a weird constraint. I don't really understand why we do install symfony & drupal for the tests. any ideas @andig ?

[andig]

For static analysis: https://github.com/php-pm/php-pm-httpkernel/blob/master/.travis.yml#L19. Need to add an update dependencies flag for that to work again.

[andig]

See https://github.com/composer/composer/issues/8518 for the ci error