Conversation
Using abumdant value objects to implement an interpreter-like machine with the name "Plan". The idea is that the plan can group the actions in a more intelligent way, that is also faster for the database to execute in most cases. Also refactored so that effects are treated more as data, this helps us compose alter statamentes into a single call when it is an option.
Implemented again rversible migrations
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #1339 +/- ##
============================================
- Coverage 75.48% 74.44% -1.04%
- Complexity 0 1771 +1771
============================================
Files 36 52 +16
Lines 4871 5268 +397
============================================
+ Hits 3677 3922 +245
- Misses 1194 1346 +152
Continue to review full report at Codecov.
|
| */ | ||
| namespace Phinx\Db\Action; | ||
|
|
||
| abstract class Action |
There was a problem hiding this comment.
Is there a convention of having an 'Abstract' prefix to abstract classes? Also,by not having a constructor, this is placing the responsibility of populating the $table on the subclass with no type hinting being enforced in this class. Making $table private and having a protected function setTable(Table $table){$this->table=$table;} would provide that enforcement of the correct type. Or having a constructor and having various tools (PHPStan for example) to identify when/if the subclass doesn't call the parent class
There was a problem hiding this comment.
I’ll add a constructor, I don’t want setters in the action classes
| class AddColumn extends Action | ||
| { | ||
|
|
||
| protected $column; |
| $this->column = $column; | ||
| } | ||
|
|
||
| public static function build(Table $table, $columnName, $type = null, $options = []) |
There was a problem hiding this comment.
Should a value object be responsible for being a factory?
There was a problem hiding this comment.
Yeah, I don’t see why not. I see it similar to overloading the constructor in java, only that php has no method overloading.
There was a problem hiding this comment.
I'd have an ActionInterface that requires each Action to be able to build itself then.
There was a problem hiding this comment.
Or an ActionFactory that builds one based upon the supplied data.
There was a problem hiding this comment.
Do you have any reason in mind for that?
There was a problem hiding this comment.
For an object whose main intention is to be passed around once created, itself acting as its own factory is redundant. Once instantiated the objects only purpose is to act as a value object. Not the one hit wonder factory.
The single responsibility aspect is what comes to mind here. Either the object represents some state OR it creates something stateful.
I may be being picky, but if the refactoring is taken in, then a very clear separation would be nicer. Smaller objects, doing fewer things. Far easier to test and understand. But as in all things, compromise between a purists perspective and a practical one.
|
This proposed, new internal architecture looks promising, not that I have an opinion that should carry any weight or, at least, not one that was solicited. I think it would be useful to implement exception handling such that custom exceptions are thrown by each “internals” component/layer (and caught by each preceding layer) flowing all the way back up to the “top level” of the migration execution “call stack”, and leading to concise and intuitive messages about the reason(s) for migration execution failure as part of migration execution as opposed to a fatal runtime error and a “mostly internals” call stack trace. Ideally these messages at the top level would be given in terms of the original/source migration, which seems particularly important with potentially multiple, intermediate forms, translations, optimizations, etc. Also, the concept of “Actions” within an “Intent” seems to open the door for an automatic, orderly unwinding of partially successful execution of a migration, particularly during development and testing. It’s always quite annoying when, for whatever reason, a failed migration results in a partially-migrated database state. |
| * | ||
| * @param string $tableName | ||
| * @param string[] $columns Column(s) | ||
| * @param string $constraint Constraint name |
Agree,. Let's do anything that seems necessary to bring Phinx to a stable & robust base for the intermediate future. |
| */ | ||
| protected $table; | ||
|
|
||
| public function __construct(Table $table) |
There was a problem hiding this comment.
Missing doc comment for function __construct()
| * @param mixed $columnName The column name | ||
| * @param mixed $type The column type | ||
| * @param mixed $options The column options | ||
| */ |
| $this->foreignKey = $fk; | ||
| } | ||
|
|
||
| public static function build(Table $table, $columns, $referencedTable, $referencedColumns = ['id'], array $options = [], $name = null) |
There was a problem hiding this comment.
Missing doc comment for function build()
| * @param Table $table The table to add the index to | ||
| * @param mixed $columns The columns to index | ||
| * @param array $options Additional options for the index creation | ||
| */ |
| * @param mixed $columnName The name of the column to change | ||
| * @param mixed $type The type of the column | ||
| * @param mixed $options Addiotional options for the column | ||
| */ |
| use Phinx\Db\Action\ChangeColumn; | ||
| use Phinx\Db\Action\DropForeignKey; | ||
| use Phinx\Db\Action\DropIndex; | ||
| use Phinx\Db\Action\DropTable; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| use Phinx\Db\Action\DropForeignKey; | ||
| use Phinx\Db\Action\DropIndex; | ||
| use Phinx\Db\Action\DropTable; | ||
| use Phinx\Db\Action\RemoveColumn; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| use Phinx\Db\Action\DropIndex; | ||
| use Phinx\Db\Action\DropTable; | ||
| use Phinx\Db\Action\RemoveColumn; | ||
| use Phinx\Db\Action\RenameColumn; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| use Phinx\Db\Action\DropTable; | ||
| use Phinx\Db\Action\RemoveColumn; | ||
| use Phinx\Db\Action\RenameColumn; | ||
| use Phinx\Db\Action\RenameTable; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| use Phinx\Db\Table\Column; | ||
| use Phinx\Db\Table\ForeignKey; | ||
| use PHPUnit\Framework\TestCase; | ||
| use Phinx\Db\Table\Table; |
There was a problem hiding this comment.
Use classes must be in alphabetical order. Was expecting PHPUnit\Framework\TestCase
|
The inspection completed: 60 new issues, 197 updated code elements |
7 participants
Adding new features to Phinx was proving difficult for myself, due to its current arquitechture.
I decided to re-implement most of the internals as an attempt to remedy the situation and open up a way to add several more improvements in the future. I'll start with the benefits I was able to extract for this new architecture:
Benefits so far
ALTERinstructions as it can doThe new architecture
Running migrations is very similar to compiling and executing a program. The executing a php file, for example, the parser will transform the instruction into an intermediate structure and then it executes it as it reads it.Optionally, php can also re-order some parts of the intermediate structure to add some optimisations.
This new architecture fits the same idea:
The problem with the old architecture was that instructions were sent immediately to the machine, instead of creating the intermediate structure. This new architecture builds an
Intent, which is then transformed and optimised in several steps before it is finally sent to the database.The new program flow is in broad terms as follows:
Intentobject containing allActionobjects to executeActionobjects are just value objects representing something to do in the databasePlanobject is used to transform theIntentinto a set of intermediate structures: EitherNewTableorAlterTableobjectsAlterTableobjects into another object calledAlterInstructionsAlterInstructionsare then compiled down to SQL and executed one by one in the database.As you may guess, each transformation opens the possibility of adding certain optimisations. During this refactoring I only added one of such optimisations: The ability to run all
ALTERinstructions for a table as a single command. This is specially beneficial forMysqlandPostgresqlBreaking changes
This is a very significant breaking change, and I guess we can have that luxury as phinx has not been marked as stable. From the user point of view, there should be no visible changes, but for anyone extending the internals, this will break their code for sure.