pomm-project / pomm-bundle

Pomm2 bundle for Symfony
81 stars 31 forks source link

Add suppor for symfony's 3.3 service locator #73

Open mvrhov opened 7 years ago

mvrhov commented 7 years ago

This adapts model and model layer poolers to the symfony 3.3 and their service locator, and a new way to define the service to be autowireable. It also gets rid of the deprecation notices.

mvrhov commented 7 years ago

Failures are not because of my PR.

mvrhov commented 7 years ago

@sanpii This one is continuation of a previous one. It would be nice to have it 2.3

sanpii commented 7 years ago

This one is continuation of a previous one. It would be nice to have it 2.3

Sorry, the 2.3 is already in release candidate phase, it’s not possible to add new feature at this point.

chanmix51 commented 7 years ago

Is it possible to add it to the 2.4 milestone then?

sanpii commented 7 years ago

Is it possible to add it to the 2.4 milestone then?

Yes, of course, except if there is a BC break.

mvrhov commented 7 years ago

This is a bit unfortunate. As SF 3.3 will be out at the end of the month and using this bundle with 3.3 will immediately result in a deprecation warnings

sanpii commented 7 years ago

This is a bit unfortunate. As SF 3.3 will be out at the end of the month and using this bundle with 3.3 will immediately result in a deprecation warnings

It’s a good excuse to make a new release quickly 😄

lunika commented 7 years ago

This is a bit unfortunate. As SF 3.3 will be out at the end of the month and using this bundle with 3.3 will immediately result in a deprecation warnings

Which one ? I updated to SF 3.3 and I didn't have deprecation warning from PommBundle.

mvrhov commented 7 years ago

Only if you use the autowiring.

lunika commented 7 years ago

Right, the autowiring-type property is deprecated.

mvrhov commented 7 years ago

@sanpii Can we get this merged into the master please. It's annoying to use a local fork.

mvrhov commented 7 years ago

So in two months time the sf 4.0 will be released and this PR is till not merged. Can we please merge this after 5 months.

sanpii commented 6 years ago

I don’t understand the complexity of this PR. For me, it should be something like this:

if (version_compare(\Symfony\Component\HttpKernel\Kernel::VERSION, '3.3', '>=')) {
    $container->setAlias($class,  $id);
} else {
    $new->addAutowiringType($class);
}
mvrhov commented 6 years ago

@sanpii Isn't running this on each loop a little bit expensive? I know that this is run once but still. I can replace the class_exists with version compare if that's what you prefer.

sanpii commented 6 years ago

Isn't running this on each loop a little bit expensive?

Yes, it’s probably a good idea.

mvrhov commented 6 years ago

Done

mvrhov commented 6 years ago

@sanpii ping. This is slowly getting annoying. I have this open for 8 moths. And it's still not merged. This bundle could have been sf4 compatible 8 moths ago.

mvrhov commented 6 years ago

@sanpii This one is rebased now and should be a part of 2.4

mvrhov commented 6 years ago

Tests seem to pass except phpcs is complaining about something but it doesn't tell about what.

stood commented 6 years ago

@mvrhov for phpcs : vendor/bin/phpcs --standard=psr2 sources

mvrhov commented 6 years ago

Fixed. Thanks