Refactor internal classes #646

jwage · 2018-05-07T04:24:37Z

Fixes #645
Fixes #629

I moved stuff around and refactored but maintained BC in the public API. There is more to do in general but to avoid a monster PR I think this is all I want to do for now.

stof · 2018-05-07T13:20:39Z

lib/Doctrine/Migrations/Configuration/Configuration.php

@@ -109,6 +113,7 @@ public function __construct(
        $this->outputWriter    = $outputWriter ?? new OutputWriter();
        $this->migrationFinder = $finder ?? new RecursiveRegexFinder();
        $this->queryWriter     = $queryWriter;
+        $this->eventDispatcher = new EventDispatcher($this, $this->connection);


having a circular object graph here is quite unfortunate. Couldn't we handle it differently ?

stof · 2018-05-07T13:21:02Z

lib/Doctrine/Migrations/EventDispatcher.php

+
+    public function dispatchEvent(string $eventName, ?EventArgs $args = null) : void
+    {
+        $this->connection->getEventManager()->dispatchEvent($eventName, $args);


instead of injecting the whole Connection, shouldn't we inject only the EventManager ?

I would not reuse EvM from Connection at all, it's entirely different layer.

Majkl578 · 2018-05-07T13:45:11Z

lib/Doctrine/Migrations/EventDispatcher.php

+use Doctrine\Migrations\Event\MigrationsEventArgs;
+use Doctrine\Migrations\Event\MigrationsVersionEventArgs;
+
+final class EventDispatcher


I am personally not a big fan of introducing yet another event dispatcher in Doctrine, given that we are looking for replacement of the one in Common for ORM 3.0.

I suppose this would be good as temporary solution until the replacement is found, but then it should be abstracted through interface + marked as interal.

@Majkl578 this is not necessarily a new "EventDispatcher", it is a composition class that depends on the existing Doctrine\Common\EventManager. I am fine with marking it internal. I don't intend for anyone else to be using this outside of migrations internals.

This is really just an abstraction on top of EventManager and not necessarily a new EventDispatcher. I will mark it as internal.

Majkl578 · 2018-05-07T13:45:51Z

lib/Doctrine/Migrations/EventDispatcher.php

+
+    public function dispatchEvent(string $eventName, ?EventArgs $args = null) : void
+    {
+        $this->connection->getEventManager()->dispatchEvent($eventName, $args);


I would not reuse EvM from Connection at all, it's entirely different layer.

Majkl578 · 2018-05-07T13:46:37Z

lib/Doctrine/Migrations/Version.php

@@ -96,7 +81,17 @@ public function __construct(

        $this->schemaProvider = $schemaProvider;

-        $this->parameterFormatter = new ParameterFormatter($this->connection);
+        $this->versionExecutor = new VersionExecutor(


This isn't DI friendly.

Majkl578 · 2018-05-07T13:47:23Z

lib/Doctrine/Migrations/Version.php

    {
-        $this->markVersion(self::DIRECTION_UP);
+        $this->state = $state;


This needs validation of $state, I think simple assert() would do.

Majkl578 · 2018-05-07T13:48:27Z

lib/Doctrine/Migrations/Version.php

-    public function getMigration() : AbstractMigration
-    {
-        return $this->migration;
+    public function exec(


No abbreviations 😭, anything wrong with execute()?

I maintained backwards compatibility. The execute() method already exists and I didn't want to change the signature of that method.

Majkl578 · 2018-05-07T13:50:33Z

lib/Doctrine/Migrations/Version.php

+        return $this->time;
+    }
+
+    public function setTime(float $time) : void


Instead of doing this, could it be moved to the Result? Or possibly passed around through some ExecutionContext?

Yes, I think we can refactor this further to get rid of all the state in the Version class.

Majkl578 · 2018-05-07T13:51:23Z

lib/Doctrine/Migrations/VersionExecutor.php

+    }
+
+    /**
+     * @param string[]|string $sql


I'd try to eliminate this by always passing an array of strings (or a collection). This leads to some nasty if-else tricks.

I can make this change here, I won't be able to make the change in the addSql() that the end user uses as it would be a big BC break, but I can make it so the internal addSql here always receives a string[] or some class/collection of queries.

stof · 2018-05-07T14:56:01Z

@Majkl578 the advantage of keeping the EventManager of the Connection is that users have a single place to register their listeners, instead of having to gain access to the specific EventManager used by the doctrine/migrations layer (which would make it harder to use doctrine/migrations only as a tool and not as a library).

Majkl578 · 2018-05-07T15:01:11Z

That is currently true for DBAL 2.x, but since Doctrine's EvM has uncertain fate, I would be more careful about making it a hard contract, given that DBAL 3 and ORM 3 may ship something different (I assume Migrations 2.0 will eventually support DBAL 3).

stof · 2018-05-07T15:10:56Z

Well, would doctrine/migrations wait for the DBAL 3 release to be released, or do you plan to release it sooner ? If you want to be able to support DBAL 3, it might be quite hard to release migrations 2.x sooner, as it might require other API changes to support it. And if we wait, we can keep this implementation until the future of EvM gets settled.

jwage · 2018-05-07T15:24:11Z

@stof I don't want to wait for DBAL 3.0. We can make another release for 3.0 at a later date once that happens.

stof · 2018-05-07T15:57:15Z

Then I vote for keeping the usage of the EvM of the connection for now, to make the upgrade to doctrine/migrations easy (listeners would work the same than in 1.x for users), and making a new major version of doctrine/migrations for DBAL 3.0 if needed, based on what the DBAL 3 support involves

jwage · 2018-05-07T18:05:46Z

@stof Agree. This is my current plan.

Majkl578 · 2018-05-07T20:10:27Z

lib/Doctrine/Migrations/VersionExecutionResult.php

+
+    public function setError(bool $error) : bool
+    {
+        $this->error = $error;


As scrutinizer points out, this will crash due to void return in non-void method.

jwage added this to the 2.0 milestone May 7, 2018

jwage self-assigned this May 7, 2018

jwage force-pushed the refactor-internal-classes branch 2 times, most recently from 2689f6c to b3c497b Compare May 7, 2018 04:56

jwage changed the title ~~[WIP] Refactor internal classes~~ Refactor internal classes May 7, 2018

jwage requested a review from Majkl578 May 7, 2018 05:09

stof reviewed May 7, 2018

View reviewed changes

Majkl578 reviewed May 7, 2018

View reviewed changes

Refactor event dispatching to EventDispatcher class.

5bd1448

jwage force-pushed the refactor-internal-classes branch 2 times, most recently from 230bd84 to b87bf47 Compare May 7, 2018 14:51

jwage force-pushed the refactor-internal-classes branch 4 times, most recently from 2bdc913 to 017676e Compare May 7, 2018 20:03

Majkl578 reviewed May 7, 2018

View reviewed changes

jwage force-pushed the refactor-internal-classes branch 6 times, most recently from 6a2f3d2 to 93253f5 Compare May 7, 2018 20:55

Refactor internal classes.

9103d17

jwage force-pushed the refactor-internal-classes branch from 93253f5 to 9103d17 Compare May 7, 2018 21:51

jwage merged commit 12f4fc8 into master May 7, 2018

Majkl578 deleted the refactor-internal-classes branch May 7, 2018 23:02

jwage added the Enhancement label May 19, 2018

jwage added Improvement and removed Enhancement labels Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor internal classes #646

Refactor internal classes #646

jwage commented May 7, 2018 •

edited

Loading

stof May 7, 2018

stof May 7, 2018

Majkl578 May 7, 2018

Majkl578 May 7, 2018

jwage May 7, 2018

jwage May 7, 2018

Majkl578 May 7, 2018

Majkl578 May 7, 2018

Majkl578 May 7, 2018

Majkl578 May 7, 2018

jwage May 7, 2018 •

edited

Loading

Majkl578 May 7, 2018

jwage May 7, 2018

Majkl578 May 7, 2018

jwage May 7, 2018 •

edited

Loading

stof commented May 7, 2018

Majkl578 commented May 7, 2018

stof commented May 7, 2018

jwage commented May 7, 2018

stof commented May 7, 2018

jwage commented May 7, 2018

Majkl578 May 7, 2018

Refactor internal classes #646

Refactor internal classes #646

Conversation

jwage commented May 7, 2018 • edited Loading

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

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

jwage May 7, 2018 • edited Loading

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

jwage May 7, 2018 • edited Loading

Choose a reason for hiding this comment

stof commented May 7, 2018

Majkl578 commented May 7, 2018

stof commented May 7, 2018

jwage commented May 7, 2018

stof commented May 7, 2018

jwage commented May 7, 2018

Choose a reason for hiding this comment

jwage commented May 7, 2018 •

edited

Loading

jwage May 7, 2018 •

edited

Loading

jwage May 7, 2018 •

edited

Loading