Make Kdyby/Events optional #266

enumag · 2016-04-17T19:10:14Z

I wanted to add some tests but unfortunately I was not even able to run the current tests - PHP just crashed. :-( Which means this is highly experimental.

enumag · 2016-04-17T19:39:00Z

Travis is failing because of some incompatibility in dev Nette.

enumag · 2016-04-17T20:19:14Z

Failing because of Kdyby/Console#54.

fprochazka · 2016-04-19T10:15:30Z

src/Kdyby/Doctrine/DI/OrmExtension.php

-
-
-
-	protected function processRepositories()


I would like to keep this as a separate method

No problem. It's just that beforeCompile will only call this method and nothing else which seems a bit odd. But sure. :-)

fprochazka · 2016-04-19T12:17:26Z

I've cherrypicked and fixed few missing occurences of ->class, 3e8bd5d. Thanks!

enumag · 2016-04-21T08:35:40Z

Travis finally passed after rebase. Which means I at least didn't break any tests. I'm still unable to run the tests on localhost though so writing a new test is not easy.

enumag · 2016-04-24T15:20:31Z

Finally managed to run the tests on localhost by using -p php (from .travis.yml).

In the end I didn't write any new tests, just changed the old ones little bit:

Replaced EventsExtension with a simple service Doctrine\Common\EventManager for all tests.
Identified tests broken in step 1 - Events.compatibility.phpt and TargetEntityMapping.phpt.
These tests do require Kdyby/Events so I refactored them to use different configuration.

enumag · 2016-04-24T15:22:22Z

@fprochazka With the tests updated I'm satisfied with this PR as it is. Can you review the changes please? :-)

enumag · 2016-05-06T10:17:23Z

Refactored Kdyby/Events detection since you recently wanted a method instead of a property in similar case in Kdyby/Console.

fprochazka · 2016-07-08T20:33:09Z

tests/KdybyTests/Doctrine/ORMTestCase.php

@@ -64,6 +69,12 @@ protected function createMemoryManager()



+	protected function configure(Configurator $config)


this is not used anywhere

[Closes #266] [Closes #239] [Ref #238]

fprochazka · 2016-07-08T20:57:21Z

@enumag Thank you, merged! I've made some minor changes

diff --git a/src/Kdyby/Doctrine/DI/OrmExtension.php b/src/Kdyby/Doctrine/DI/OrmExtension.php
index 7155fd4..0da259f 100644
--- a/src/Kdyby/Doctrine/DI/OrmExtension.php
+++ b/src/Kdyby/Doctrine/DI/OrmExtension.php
@@ -351,24 +351,26 @@ class OrmExtension extends Nette\DI\CompilerExtension
                        $this->targetEntityMappings = Nette\Utils\Arrays::mergeTree($this->targetEntityMappings, $config['targetEntityMappings']);
                }

-               $entityManagerArguments = [
-                       $connectionService = $this->processConnection($name, $defaults, $isDefault),
-                       $this->prefix('@' . $name . '.ormConfiguration'),
-               ];
-
                if ($this->isKdybyEventsPresent()) {
                        $builder->addDefinition($this->prefix($name . '.evm'))
                                ->setClass('Kdyby\Events\NamespacedEventManager', [Kdyby\Doctrine\Events::NS . '::'])
                                ->addSetup('$dispatchGlobalEvents', [TRUE]) // for BC
                                ->setAutowired(FALSE);

-                       $entityManagerArguments[] = $this->prefix('@' . $name . '.evm');
+               } else {
+                       $builder->addDefinition($this->prefix($name . '.evm'))
+                               ->setClass('Doctrine\Common\EventManager')
+                               ->setAutowired(FALSE);
                }

                // entity manager
                $entityManager = $builder->addDefinition($managerServiceId = $this->prefix($name . '.entityManager'))
                        ->setClass('Kdyby\Doctrine\EntityManager')
-                       ->setFactory('Kdyby\Doctrine\EntityManager::create', $entityManagerArguments)
+                       ->setFactory('Kdyby\Doctrine\EntityManager::create', [
+                               $connectionService = $this->processConnection($name, $defaults, $isDefault),
+                               $this->prefix('@' . $name . '.ormConfiguration'),
+                               $this->prefix('@' . $name . '.evm'),
+                       ])
                        ->addTag(self::TAG_ENTITY_MANAGER)
                        ->addTag('kdyby.doctrine.entityManager')
                        ->setAutowired($isDefault)
@@ -523,19 +525,13 @@ class OrmExtension extends Nette\DI\CompilerExtension

                // connection
                $options = array_diff_key($config, array_flip(['types', 'resultCache', 'connection', 'logging']));
-
-               $connectionArguments = [
-                       $options,
-                       $this->prefix('@' . $name . '.dbalConfiguration'),
-               ];
-
-               if ($this->isKdybyEventsPresent()) {
-                       $connectionArguments[] = $this->prefix('@' . $name . '.evm');
-               }
-
                $connection = $builder->addDefinition($connectionServiceId = $this->prefix($name . '.connection'))
                        ->setClass('Kdyby\Doctrine\Connection')
-                       ->setFactory('Kdyby\Doctrine\Connection::create', $connectionArguments)
+                       ->setFactory('Kdyby\Doctrine\Connection::create', [
+                               $options,
+                               $this->prefix('@' . $name . '.dbalConfiguration'),
+                               $this->prefix('@' . $name . '.evm'),
+                       ])
                        ->addSetup('setSchemaTypes', [$schemaTypes])
                        ->addSetup('setDbalTypes', [$dbalTypes])
                        ->addTag(self::TAG_CONNECTION)
diff --git a/tests/KdybyTests/Doctrine/ORMTestCase.php b/tests/KdybyTests/Doctrine/ORMTestCase.php
index 0d6a521..b6d0f4f 100644
--- a/tests/KdybyTests/Doctrine/ORMTestCase.php
+++ b/tests/KdybyTests/Doctrine/ORMTestCase.php
@@ -69,12 +69,6 @@ abstract class ORMTestCase extends Tester\TestCase



-       protected function configure(Configurator $config)
-       {
-       }
-
-
-
        /**
         * @param string $className
         * @param array $props
diff --git a/tests/KdybyTests/nette-reset.neon b/tests/KdybyTests/nette-reset.neon
index 0e29325..cc18417 100644
--- a/tests/KdybyTests/nette-reset.neon
+++ b/tests/KdybyTests/nette-reset.neon
@@ -31,5 +31,3 @@ session:
 services:
        cacheStorage:
                class: Nette\Caching\Storages\MemoryStorage
-       evm:
-               class: Doctrine\Common\EventManager

enumag · 2016-07-08T21:14:15Z

If I'm not mistaken those minor changes break what I intended - I need the evm to be autowired, not use some dummy replacement

fprochazka · 2016-07-08T21:18:38Z

I know, but I don't want the users to have to configure some magic service, that should be configured by Kdyby.
If you wanna provide custom implementation of Doctrine\Common\EventManager, you can override the evm service for the EM.

Now you can

use this with Kdyby/Events and it works
use this without Kdyby/Events and it works
use this without Kdyby/Events, with custom implementation of EventManager and it works.

IMHO it's cleaner that way and it makes sense that the third requires you to touch the evm service in config.

enumag · 2016-07-08T21:25:46Z

I can't agree. I have an extension providing a simple ContainerAwareEventManager and wanted to use it in Kdyby Doctrine. However my extension should not be aware of Kdyby/Doctrine at all (it's independent) - you're forcing me to specifically look for OrmExtension and hack it. That's not clean at all.

fprochazka · 2016-07-08T21:38:00Z

First of all

I don't want the users to have to configure some magic service, that should be configured by Kdyby.

I hope you understand that. I wanna avoid this at all costs.

By just simply accepting what you've proposed, It would be simple for you, but I'd cripple the UX of the lib for everyone else.

About your argument - I don't consider it hacking. But I understand why you don't like it.

What about adding another case - if Kdyby/Events is not registered and a service of type Doctrine\Common\EventManager can be found in config, the evm service will be an alias to that service.

enumag · 2016-07-08T21:39:06Z

How about adding the evm service only if there is no autowired EventManager?

EDIT: Well that alias means pretty much the same thing so I don't care which one of these two solutions we use.

fprochazka · 2016-07-08T21:44:32Z

Imho it would be better to do something like

$evm = $builder->addDefinition($this->prefix($name . '.evm'))
    ->setClass('Doctrine\Common\EventManager')
    ->setAutowired(FALSE);

if ($customEvmService = $builder->getByType('Doctrine\Common\EventManager')) {
    $evm->setFactory('@' . $customEvmService);
}

but that's a detail :)

enumag · 2016-07-08T21:45:54Z

@fprochazka Yeah. This is fine by me. :-)

fprochazka · 2016-07-08T21:46:13Z

My point is that you don't have to introduce more if's in the rest of the code :)

enumag · 2016-07-08T21:47:38Z

I'm aware... I didn't like the ifs in my PR either. :-)

fprochazka · 2016-07-08T21:47:39Z

I'm lazy to write a test for it right now. You mind doing it and sending a PR?

enumag · 2016-07-08T21:49:36Z

Well... You're well aware that I won't refuse. :-D

enumag force-pushed the events branch from 2bab7dd to 823fbae Compare April 17, 2016 19:35

enumag force-pushed the events branch from bde1a7a to b5a9798 Compare April 17, 2016 21:19

fprochazka reviewed Apr 19, 2016
View reviewed changes

enumag force-pushed the events branch 2 times, most recently from 18701c1 to eff881f Compare April 20, 2016 20:06

Make Kdyby/Events optional

c5f320a

enumag force-pushed the events branch from eff881f to c5f320a Compare April 21, 2016 08:28

enumag mentioned this pull request Apr 23, 2016

Chop up the package #238

Open

31 tasks

Update tests

9d52d7f

enumag mentioned this pull request Apr 26, 2016

Add ContainerAwareEventManager as service if needed Arachne/Doctrine#2

Closed

Add method isKdybyEventsPresent

e694885

fprochazka reviewed Jul 8, 2016
View reviewed changes

fprochazka pushed a commit that referenced this pull request Jul 8, 2016

Make Kdyby/Events optional

be31ae2

[Closes #266] [Closes #239] [Ref #238]

fprochazka pushed a commit that referenced this pull request Jul 8, 2016

Make Kdyby/Events optional

9dd6673

[Closes #266] [Closes #239] [Ref #238]

fprochazka closed this in 55a7714 Jul 8, 2016

enumag mentioned this pull request Jul 9, 2016

Use autowired EventManager if available #278

Merged

enumag mentioned this pull request Jun 1, 2017

Join contributte Arachne/EventManager#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Kdyby/Events optional #266

Make Kdyby/Events optional #266

enumag commented Apr 17, 2016 •

edited

Loading

enumag commented Apr 17, 2016

enumag commented Apr 17, 2016

fprochazka Apr 19, 2016

enumag Apr 19, 2016

fprochazka commented Apr 19, 2016

enumag commented Apr 21, 2016

enumag commented Apr 24, 2016

enumag commented Apr 24, 2016

enumag commented May 6, 2016

fprochazka Jul 8, 2016

fprochazka commented Jul 8, 2016

enumag commented Jul 8, 2016

fprochazka commented Jul 8, 2016 •

edited

Loading

enumag commented Jul 8, 2016 •

edited

Loading

fprochazka commented Jul 8, 2016

enumag commented Jul 8, 2016 •

edited

Loading

fprochazka commented Jul 8, 2016 •

edited

Loading

enumag commented Jul 8, 2016

fprochazka commented Jul 8, 2016

enumag commented Jul 8, 2016

fprochazka commented Jul 8, 2016

enumag commented Jul 8, 2016

		@@ -64,6 +69,12 @@ protected function createMemoryManager()



		protected function configure(Configurator $config)

Make Kdyby/Events optional #266

Make Kdyby/Events optional #266

Conversation

enumag commented Apr 17, 2016 • edited Loading

enumag commented Apr 17, 2016

enumag commented Apr 17, 2016

fprochazka Apr 19, 2016

Choose a reason for hiding this comment

enumag Apr 19, 2016

Choose a reason for hiding this comment

fprochazka commented Apr 19, 2016

enumag commented Apr 21, 2016

enumag commented Apr 24, 2016

enumag commented Apr 24, 2016

enumag commented May 6, 2016

fprochazka Jul 8, 2016

Choose a reason for hiding this comment

fprochazka commented Jul 8, 2016

enumag commented Jul 8, 2016

fprochazka commented Jul 8, 2016 • edited Loading

enumag commented Jul 8, 2016 • edited Loading

fprochazka commented Jul 8, 2016

enumag commented Jul 8, 2016 • edited Loading

fprochazka commented Jul 8, 2016 • edited Loading

enumag commented Jul 8, 2016

fprochazka commented Jul 8, 2016

enumag commented Jul 8, 2016

fprochazka commented Jul 8, 2016

enumag commented Jul 8, 2016

enumag commented Apr 17, 2016 •

edited

Loading

fprochazka commented Jul 8, 2016 •

edited

Loading

enumag commented Jul 8, 2016 •

edited

Loading

enumag commented Jul 8, 2016 •

edited

Loading

fprochazka commented Jul 8, 2016 •

edited

Loading