Factory service test #85

silvioq · 2017-11-19T01:04:35Z

Hi, I hope this pull request was useful for you. I apologize for my basic English

  # test example ...
  $this->assertContainerBuilderHasCreatedByFactoryService(
      'target.service', 'factory.service', 'factoryMethod');

silvioq · 2017-11-19T01:35:32Z

Sadly, factories aren't compatible with Symfony 2.3 ... Do you plan remove BC for that version?

xabbuh · 2017-11-20T09:53:05Z

PhpUnit/ContainerBuilderHasFactoryConstraint.php

+
+        $factory = $definition->getFactory();
+
+        if( !is_array( $factory ) ) {


You could check if the Definition::getFactoryClass() method exists to implement a pre Symfony 2.7 compatible solution.

- Check for <2.6 symfony/dependency-injection version - Test old yml syntax format

silvioq · 2017-11-20T22:46:26Z

PhpUnit/ContainerBuilderHasFactoryConstraint.php

@@ -175,4 +176,30 @@ private function evaluateFactoryClass(ContainerBuilder $containerBuilder, $retur
        return true;
    }

+    private function getFactoryData( Definition $definition )
+    {
+        if( self::isLegacySymfonyDI() ) {


This is a dirty, but working solution ... I checked with DI 2.3, 3.0 and 3.3 and they working ... a better approach may be an abstract class with "getFactoryData" as abstract method, but this commit resolves <= 2.6 versions and variated factory syntax in yml.

A test case not cover is a service with "setFactoryClass" definition ... This is pending.

xabbuh · 2017-11-21T08:27:15Z

PhpUnit/ContainerBuilderHasFactoryConstraint.php

+
+    static public function isLegacySymfonyDI()
+    {
+        return method_exists( new Definition(), 'getFactoryService' );


no need to create an instance here, just using the class name works too

stloyd

CS must match the Symfony CS.

stloyd · 2017-11-30T11:07:07Z

PhpUnit/ContainerBuilderHasFactoryConstraint.php

+
+    public function toString()
+    {
+        if( null === $this->expectedFactoryClass )


Please remove all spaces between sprintf() & strings inside:

if (null === $this->expectedFactoryClass) { return sprintf('"%s" has factory', $this->serviceId); }

stloyd · 2017-11-30T11:07:43Z

PhpUnit/ContainerBuilderHasFactoryConstraint.php

+
+        $factory = $this->getFactoryData($definition);
+
+        if( !is_array( $factory ) ) {


if (!is_array($factory)) {

stloyd · 2017-11-30T11:08:19Z

PhpUnit/ContainerBuilderHasFactoryConstraint.php

+
+        $factory = $this->getFactoryData( $definition );
+
+        list( $factoryDefinition, $factoryMethod ) = $factory;


Please remove all spaces in function calls. Same for those in if/else etc.

stloyd · 2017-11-30T11:08:40Z

PhpUnit/ContainerBuilderHasFactoryConstraint.php

+                return null;
+
+            return array( $factoryClass ? $factoryClass : $factoryService, $factoryMethod );
+        } else {


No point for else as you do return in if.

Nyholm · 2017-12-02T13:41:44Z

Could you also rebase on master?

- Check for <2.6 symfony/dependency-injection version - Test old yml syntax format

…nTest

silvioq · 2017-12-02T13:52:52Z

@Nyholm I'm sorry ... I'm doing some bad. How to I must rebase my PR?

silvioq · 2017-12-02T14:39:46Z

Tests/Fixtures/MatthiasDependencyInjectionTestExtension.php

@@ -31,6 +45,22 @@ public function load(array $config, ContainerBuilder $container)

        // add an alias to an existing service
        $container->setAlias('manual_alias', 'service_id');
+
+        // add an factory service
+        $container->register('manual_factory_service', new Definition());


@Nyholm I'm sorry again! The bug is here ... must be $container->register('manual_factory_service', 'stdClass');
I will fix and I will push again later.

silvioq · 2018-02-17T12:35:38Z

Hi @stloyd In December 2017 I reviewed the Symfony CS unmatches. I forgot something ?

xabbuh · 2018-02-20T09:59:39Z

PhpUnit/AbstractContainerBuilderTestCase.php

+     * @param $expectedFactoryClass
+     * @param $expectedFactoryMethod
+     */
+    protected function assertContainerBuilderHasCreatedByFactoryService($serviceId, $expectedFactoryClass = null, $expectedFactoryMethod)


now that the library requires PHP 7 we can use scalar type hints here

xabbuh · 2018-02-20T09:59:51Z

PhpUnit/ContainerBuilderHasFactoryConstraint.php

+    private $expectedFactoryClass;
+    private $expectedFactoryMethod;
+
+    public function __construct($serviceId, $expectedFactoryClass = null, $expectedFactoryMethod = null)


silvioq added 3 commits November 18, 2017 21:43

Factory test

312de09

Fix typo

7aacdbb

BC for SYMFONY_DI_VERSION=2.7|2.8|3.2

e49fdd3

xabbuh reviewed Nov 20, 2017

View reviewed changes

Backward compatibility

384d88f

- Check for <2.6 symfony/dependency-injection version - Test old yml syntax format

silvioq commented Nov 20, 2017

View reviewed changes

xabbuh reviewed Nov 21, 2017

View reviewed changes

Optimize, don't create an instance

4c7905e

stloyd requested changes Nov 30, 2017

View reviewed changes

silvioq added 2 commits December 2, 2017 10:37

Fix a lot of CS breaks

16b4ece

Merge remote-tracking branch 'upstream/master'

323330e

silvioq added 7 commits December 2, 2017 10:45

Factory test

a84f713

Fix typo

c138ec9

BC for SYMFONY_DI_VERSION=2.7|2.8|3.2

268b680

Backward compatibility

9802ce7

- Check for <2.6 symfony/dependency-injection version - Test old yml syntax format

Optimize, don't create an instance

6d228af

Fix a lot of CS breaks

569d9af

Merge branch 'master' of github.com:silvioq/SymfonyDependencyInjectio…

eb5bfec

…nTest

silvioq commented Dec 2, 2017

View reviewed changes

silvioq added 2 commits December 2, 2017 11:47

Changes legacy factory service interface to avoid deprecation messages

1c71370

Fix service creation

3fb9b74

stloyd approved these changes Feb 17, 2018

View reviewed changes

xabbuh reviewed Feb 20, 2018

View reviewed changes

Merge branch 'master' into master

5e84b5b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factory service test #85

Factory service test #85

silvioq commented Nov 19, 2017 •

edited

Loading

silvioq commented Nov 19, 2017

xabbuh Nov 20, 2017

silvioq Nov 20, 2017

xabbuh Nov 21, 2017

stloyd left a comment

stloyd Nov 30, 2017

stloyd Nov 30, 2017

stloyd Nov 30, 2017

stloyd Nov 30, 2017

Nyholm commented Dec 2, 2017

silvioq commented Dec 2, 2017

silvioq Dec 2, 2017

silvioq commented Feb 17, 2018

xabbuh Feb 20, 2018

xabbuh Feb 20, 2018


		$factory = $definition->getFactory();

		if( !is_array( $factory ) ) {


		$factory = $this->getFactoryData($definition);

		if( !is_array( $factory ) ) {


		$factory = $this->getFactoryData( $definition );

		list( $factoryDefinition, $factoryMethod ) = $factory;

Factory service test #85

Are you sure you want to change the base?

Factory service test #85

Conversation

silvioq commented Nov 19, 2017 • edited Loading

silvioq commented Nov 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stloyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Dec 2, 2017

silvioq commented Dec 2, 2017

Choose a reason for hiding this comment

silvioq commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvioq commented Nov 19, 2017 •

edited

Loading