Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add suppor for symfony's 3.3 service locator #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add suppor for symfony's 3.3 service locator #73

wants to merge 2 commits into from

Conversation

mvrhov
Copy link
Contributor

@mvrhov mvrhov commented Apr 22, 2017

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
Copy link
Contributor Author

mvrhov commented Apr 22, 2017

Failures are not because of my PR.

@mvrhov
Copy link
Contributor Author

mvrhov commented May 2, 2017

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

@sanpii
Copy link
Member

sanpii commented May 2, 2017

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
Copy link
Member

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

@sanpii
Copy link
Member

sanpii commented May 2, 2017

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

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

@mvrhov
Copy link
Contributor Author

mvrhov commented May 2, 2017

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
Copy link
Member

sanpii commented May 2, 2017

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 😄

@mvrhov mvrhov mentioned this pull request Jun 1, 2017
Closed
@mvrhov mvrhov force-pushed the modelAsServiceSf33 branch 2 times, most recently from 7868690 to edf7678 Compare June 1, 2017 07:53
@lunika
Copy link
Contributor

lunika commented Jun 1, 2017

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
Copy link
Contributor Author

mvrhov commented Jun 1, 2017

Only if you use the autowiring.

@lunika
Copy link
Contributor

lunika commented Jun 2, 2017

Right, the autowiring-type property is deprecated.

sanpii
sanpii requested changes Jun 8, 2017
View reviewed changes
sources/lib/Model/ContainerModelPooler.php
@@ -10,8 +10,8 @@
namespace PommProject\PommBundle\Model;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t undestand the goal of changes in this file. The ContainerAwareTrait already requires a ContainerInterface.

Copy link
Contributor Author

@mvrhov mvrhov Jun 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContainerAware is not compatible with PSR container. And service locator in Sf3.3 returns PSR container interface, Previous sf version their own.

sources/lib/Model/ContainerModelLayerPooler.php
@@ -11,8 +11,8 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t undestand the goal of changes in this file. The ContainerAwareTrait already requires a ContainerInterface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

sources/lib/DependencyInjection/Compiler/ModelPass.php Outdated
@@ -30,6 +30,9 @@ private function addTagged(DI\ContainerBuilder $container, $tag, $defaultService
/** @var DI\Definition[] $definitions */
$definitions = [];

$poolerToService = [];
$gteSf33 = class_exists('Symfony\Component\DependencyInjection\Compiler\ServiceLocatorTagPass');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use more explicit variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion

sources/lib/DependencyInjection/Compiler/ModelPass.php Outdated
;

if ($gteSf33) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s a good idea to split this method into two functions to reduce complexity and length of this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO there is no need.

lunika
lunika suggested changes Jun 8, 2017
View reviewed changes
sources/lib/Model/ContainerModelLayerPooler.php Outdated
use Symfony\Component\DependencyInjection\ContainerAwareInterface;
use Symfony\Component\DependencyInjection\ContainerAwareTrait;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Psr\Container\ContainerInterface as PsrContainer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should rename it in PsrContainerInterface to do like in symfony code.

sources/lib/Model/ContainerModelLayerPooler.php Outdated
/**
* @param PsrContainer|ContainerInterface $container
*/
public function setContainer($container)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can type hint with the PsrContainerInterface, The ContainerInterface extends it so you can inject both without the if block below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I cannot. Because we need to be compatible with sf < 3.3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed, I didn't thought about that... sorry

@mvrhov mvrhov force-pushed the modelAsServiceSf33 branch from edf7678 to 9995cc2 Compare July 26, 2017 16:32
@mvrhov
Copy link
Contributor Author

mvrhov commented Jul 26, 2017

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

@mvrhov
Copy link
Contributor Author

mvrhov commented Sep 26, 2017

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.

@mvrhov mvrhov mentioned this pull request Sep 26, 2017
Merged
@sanpii
Copy link
Member

sanpii commented Oct 31, 2017

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
Copy link
Contributor Author

mvrhov commented Oct 31, 2017 •
edited
Loading

@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
Copy link
Member

sanpii commented Oct 31, 2017

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

Yes, it’s probably a good idea.

@mvrhov mvrhov force-pushed the modelAsServiceSf33 branch from 9995cc2 to e1f308d Compare October 31, 2017 13:37
@mvrhov
Copy link
Contributor Author

mvrhov commented Oct 31, 2017

Done

@sanpii sanpii mentioned this pull request Dec 3, 2017
Closed
5 tasks
@mvrhov
Copy link
Contributor Author

mvrhov commented Dec 13, 2017 •
edited
Loading

@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 mvrhov force-pushed the modelAsServiceSf33 branch 2 times, most recently from f37f096 to 57b6999 Compare December 14, 2017 16:31
@mvrhov mvrhov force-pushed the modelAsServiceSf33 branch from 57b6999 to 45f9b1a Compare January 17, 2018 17:58
@mvrhov
Copy link
Contributor Author

mvrhov commented Jan 17, 2018

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

@mvrhov
Copy link
Contributor Author

mvrhov commented Jan 17, 2018 •
edited
Loading

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

@stood
Copy link
Member

stood commented Jan 17, 2018

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

@mvrhov mvrhov force-pushed the modelAsServiceSf33 branch from 45f9b1a to 5ccad45 Compare January 18, 2018 06:37
@mvrhov
Copy link
Contributor Author

mvrhov commented Jan 18, 2018

Fixed. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunika lunika lunika requested changes

@sanpii sanpii sanpii requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mvrhov @sanpii @chanmix51 @lunika @stood