-
Notifications
You must be signed in to change notification settings - Fork 120
Add support for database views and triggers #957
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.
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
base: 5.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds first-class support for creating and managing database views and triggers in CakePHP migrations, eliminating the need for raw SQL execution. The implementation introduces new value objects (View, Trigger), action classes (CreateView, DropView, CreateTrigger, DropTrigger), and integrates them into the existing migration infrastructure with database-specific SQL generation for MySQL, PostgreSQL, SQLite, and SQL Server.
Key Changes
- Added value objects and action classes for views and triggers
- Extended
AbstractAdapter,Table, andBaseMigrationwith view/trigger support - Implemented database-specific SQL generation in all four adapters (MySQL, PostgreSQL, SQLite, SQL Server)
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Db/Table/View.php |
New value object representing database views with name, definition, replace, and materialized flags |
src/Db/Table/Trigger.php |
New value object representing database triggers with timing, events, and definition |
src/Db/Action/CreateView.php |
Action class for creating views |
src/Db/Action/DropView.php |
Action class for dropping views |
src/Db/Action/CreateTrigger.php |
Action class for creating triggers |
src/Db/Action/DropTrigger.php |
Action class for dropping triggers |
src/Db/Adapter/AbstractAdapter.php |
Added abstract methods for view/trigger operations and integrated action execution |
src/Db/Adapter/MysqlAdapter.php |
MySQL-specific implementation with OR REPLACE support |
src/Db/Adapter/PostgresAdapter.php |
PostgreSQL implementation with materialized views and EXECUTE FUNCTION syntax |
src/Db/Adapter/SqliteAdapter.php |
SQLite implementation with BEGIN...END blocks |
src/Db/Adapter/SqlserverAdapter.php |
SQL Server implementation with IF OBJECT_ID checks |
src/Db/Table.php |
Added createView, dropView, createTrigger, dropTrigger methods |
src/BaseMigration.php |
Added convenience methods for easy migration usage |
tests/TestCase/Db/Adapter/MysqlAdapterTest.php |
Test coverage for view and trigger operations |
tests/TestCase/Db/Adapter/DefaultAdapterTrait.php |
Trait implementation with empty instruction methods |
docs/examples/ViewsAndTriggersExample.php |
Comprehensive usage examples and documentation |
Comments suppressed due to low confidence (1)
src/Db/Table.php:912
- The
executeActionsmethod unconditionally adds aCreateTableaction when the table doesn't exist (lines 906-907), which is incorrect for view and trigger operations. When onlyCreateView,DropView,CreateTrigger, orDropTriggeractions are present, no table should be created. Add a check to determine if only view/trigger actions exist, and skip adding theCreateTableaction in those cases. For example, add another loop after theRenameTablecheck to set$exists = trueif anyCreateView,DropView,CreateTrigger, orDropTriggeractions are found.
protected function executeActions(bool $exists): void
{
// Renaming a table is tricky, specially when running a reversible migration
// down. We will just assume the table already exists if the user commands a
// table rename.
if (!$exists) {
foreach ($this->actions->getActions() as $action) {
if ($action instanceof RenameTable) {
$exists = true;
break;
}
}
}
// If the table does not exist, the last command in the chain needs to be
// a CreateTable action.
if (!$exists) {
$this->actions->addAction(new CreateTable($this->table));
}
$plan = new Plan($this->actions);
$plan->execute($this->getAdapter());
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I have not yet needed this but LGTM 👍🏻 |
markstory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I had one question but it isn't a blocker.
| $hasTableActions = false; | ||
| $hasViewOrTriggerActions = false; | ||
|
|
||
| foreach ($actions as $action) { | ||
| if ( | ||
| $action instanceof CreateView | ||
| || $action instanceof DropView | ||
| || $action instanceof CreateTrigger | ||
| || $action instanceof DropTrigger | ||
| ) { | ||
| $hasViewOrTriggerActions = true; | ||
| } else { | ||
| $hasTableActions = true; | ||
| } | ||
| } | ||
|
|
||
| // Only skip CreateTable if we have ONLY view/trigger actions (and at least one) | ||
| if (!$hasViewOrTriggerActions || $hasTableActions || count($actions) === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are views/triggers different than columns indexes, and constraints? All of these objects require the table to exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Views and triggers are potentially fundamentally different from columns, indexes, and constraints in that they are standalone database objects, not properties of a table.
Columns, indexes, constraints:
- These belong to a table - you can't have a column without a table to put it in
- They're created with ALTER TABLE or as part of CREATE TABLE
- They cannot exist independently
Views:
- A view is created with CREATE VIEW view_name AS SELECT... - it's a separate database object
- Views don't "belong" to any table - they can query multiple tables or even no tables at all
- The Table class here is just used as a convenience API to access the adapter infrastructure
Triggers:
- While triggers do reference a table (the one they fire on), they're still separate database objects
- They're created with CREATE TRIGGER trigger_name ON table_name...
- The referenced table must already exist, but we're not adding the trigger as part of creating that table
The $this->table('view_name') call is essentially creating a "dummy" Table instance just to access the action/adapter pipeline. If we didn't skip CreateTable, calling $this->createView('active_users', '...') would try to create a table named "active_users" before creating the view - which would fail since the table and view would have the same name.
What would you propose here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Views don't "belong" to any table - they can query multiple tables or even no tables at all
But those view do require the tables to exist already? Or does the exists parameter not reflect whether or not the table already exists in the database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we maintained an outer array of "action groups" keyed by the object name, that way when we detect the start of a new database object we can cluster its actions under a different key. Then we flatten the array and execute all the actions as normal.
$objectActions = [
'table1' => [ Action1, Action2 ],
'view1' => [ Action3, Action4, Action 5 ],
];
Although, I realize that users can put the actions in any order, so is that going to make it difficult to keep track of which actions go with which object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A slightly different interface that might be a bit cleaner:
$tableActions = $this->actions->getTableActions();
$viewActions = $this->actions->getViewActions();
$triggerActions = $this->actions->getTriggerActions();
// ...etc.
This moves the action-type detection logic to the Intent (?) class, which I admit I have not inspected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $exists parameter reflects whether the table with the view name exists. When calling $this->createView('active_users', '...'), it internally does $this->table('active_users') - creating a Table object with that name. If we didn't skip CreateTable, it would try to create a table named "active_users" before creating the view - which would fail since you can't have both a table and view with the same name.
The tables referenced inside the view's SELECT must exist, yes - but that's the user's responsibility to ensure proper ordering in their migrations. The $exists check here is specifically about the "container" table object, not the tables the view queries.
This implements support for creating and managing database views and triggers through CakePHP migrations, addressing issue #347. - Create and drop database views in migrations - Support for OR REPLACE syntax (MySQL, PostgreSQL) - Materialized views support (PostgreSQL only) - Database-agnostic API with adapter-specific implementations - Create and drop database triggers in migrations - Support for BEFORE/AFTER/INSTEAD OF timing - Support for INSERT/UPDATE/DELETE events - Support for multiple events per trigger - FOR EACH ROW vs FOR EACH STATEMENT options **Value Objects:** - `Migrations\Db\Table\View` - Represents a database view - `Migrations\Db\Table\Trigger` - Represents a database trigger **Actions:** - `Migrations\Db\Action\CreateView` - Action for creating views - `Migrations\Db\Action\DropView` - Action for dropping views - `Migrations\Db\Action\CreateTrigger` - Action for creating triggers - `Migrations\Db\Action\DropTrigger` - Action for dropping triggers **Core:** - `AbstractAdapter` - Added abstract methods for view/trigger support - `Table` - Added createView(), dropView(), createTrigger(), dropTrigger() - `BaseMigration` - Added convenience methods for easy migration usage **Adapters:** - `MysqlAdapter` - MySQL-specific view/trigger SQL generation - `PostgresAdapter` - PostgreSQL implementation with materialized views - `SqliteAdapter` - SQLite-specific syntax handling - `SqlserverAdapter` - SQL Server implementation ```php // Create a view $this->createView( 'active_users', 'SELECT * FROM users WHERE status = "active"' ); // Create a materialized view (PostgreSQL) $this->createView( 'user_stats', 'SELECT user_id, COUNT(*) FROM posts GROUP BY user_id', ['materialized' => true] ); // Create a trigger $this->createTrigger( 'users', 'log_changes', 'INSERT', "INSERT INTO audit_log VALUES (NEW.id, NOW())", ['timing' => 'AFTER'] ); // Drop view/trigger $this->dropView('active_users'); $this->dropTrigger('users', 'log_changes'); ``` Added comprehensive tests for view and trigger functionality in MysqlAdapterTest covering creation, querying, and deletion. Added example migration file demonstrating various view and trigger scenarios with database-specific considerations. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…er methods The anonymous test class extending AbstractAdapter needs to implement the new abstract methods for view and trigger support.
2d4a205 to
8153f25
Compare
- Add missing `use Exception` and `use Migrations\Db\Table` imports to AbstractAdapter - Add `: void` return types to algorithm/lock test methods in MysqlAdapterTest - Fix alphabetical ordering of use statements 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
8153f25 to
64ef747
Compare
|
Since it is possible to use the Table API to create a view, can you also pass it a SelectQuery object from the table? |
I vote we can add this later if it is needed. The fact that we are covering the base use cases for four different database engines is a huge win for now. If the community needs these later, we can add them.
Personally, no. I like the pattern that we have adopted with the |
jamisonbryant
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but I don't use views/triggers much. I had one question and a few small suggestions, but also nothing blocking. Therefore, I will approve.
| return [ | ||
| $this->constraints, | ||
| $this->tableMoves, | ||
| $this->viewsAndTriggers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: This is the first place in this branch that we are combining views/triggers elements into one symbol. Up until now, they've been separate classes/methods. Why the change?
I asked Claude "how similar are these two concepts" to see if there was something I was missing but it said the overlap between them is pretty thin. Should we break this into two properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're grouped because they share the same execution ordering requirement in the plan - both run after constraints and table moves. Splitting them would add complexity without functional benefit since they don't conflict. But I can split them if you feel strongly about it.
| $hasTableActions = false; | ||
| $hasViewOrTriggerActions = false; | ||
|
|
||
| foreach ($actions as $action) { | ||
| if ( | ||
| $action instanceof CreateView | ||
| || $action instanceof DropView | ||
| || $action instanceof CreateTrigger | ||
| || $action instanceof DropTrigger | ||
| ) { | ||
| $hasViewOrTriggerActions = true; | ||
| } else { | ||
| $hasTableActions = true; | ||
| } | ||
| } | ||
|
|
||
| // Only skip CreateTable if we have ONLY view/trigger actions (and at least one) | ||
| if (!$hasViewOrTriggerActions || $hasTableActions || count($actions) === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we maintained an outer array of "action groups" keyed by the object name, that way when we detect the start of a new database object we can cluster its actions under a different key. Then we flatten the array and execute all the actions as normal.
$objectActions = [
'table1' => [ Action1, Action2 ],
'view1' => [ Action3, Action4, Action 5 ],
];
Although, I realize that users can put the actions in any order, so is that going to make it difficult to keep track of which actions go with which object?
| $hasTableActions = false; | ||
| $hasViewOrTriggerActions = false; | ||
|
|
||
| foreach ($actions as $action) { | ||
| if ( | ||
| $action instanceof CreateView | ||
| || $action instanceof DropView | ||
| || $action instanceof CreateTrigger | ||
| || $action instanceof DropTrigger | ||
| ) { | ||
| $hasViewOrTriggerActions = true; | ||
| } else { | ||
| $hasTableActions = true; | ||
| } | ||
| } | ||
|
|
||
| // Only skip CreateTable if we have ONLY view/trigger actions (and at least one) | ||
| if (!$hasViewOrTriggerActions || $hasTableActions || count($actions) === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A slightly different interface that might be a bit cleaner:
$tableActions = $this->actions->getTableActions();
$viewActions = $this->actions->getViewActions();
$triggerActions = $this->actions->getTriggerActions();
// ...etc.
This moves the action-type detection logic to the Intent (?) class, which I admit I have not inspected.
|
@jamisonbryant Re: SelectQuery - Not currently, it takes a raw SQL string. We could potentially add SelectQuery support in a future enhancement, but it would add complexity since the ORM's query builder isn't designed for the migration context (different connections, potentially different table structures during migration). The raw SQL approach keeps it simple and database-agnostic for now. |
Description
This PR implements support for creating and managing database views and triggers through CakePHP migrations, addressing issue #347.
Previously, users had to resort to raw SQL execution to create views and triggers, which undermined the abstraction that migrations provide. This PR adds first-class support for these database objects with a clean, database-agnostic API.
Changes
New Features
Views
OR REPLACEsyntax (MySQL, PostgreSQL)Triggers
BEFORE/AFTER/INSTEAD OFtimingINSERT/UPDATE/DELETEevents (single or multiple)FOR EACH ROWvsFOR EACH STATEMENToptionsNew Classes
Value Objects:
Migrations\Db\Table\View- Represents a database view with properties like name, definition, replace flag, and materialized flagMigrations\Db\Table\Trigger- Represents a database trigger with timing, events, and definitionAction Classes:
Migrations\Db\Action\CreateView- Action for creating viewsMigrations\Db\Action\DropView- Action for dropping viewsMigrations\Db\Action\CreateTrigger- Action for creating triggersMigrations\Db\Action\DropTrigger- Action for dropping triggersModified Classes
Core Infrastructure:
AbstractAdapter- Added abstract methodsgetCreateViewInstructions(),getDropViewInstructions(),getCreateTriggerInstructions(),getDropTriggerInstructions()and integrated them into the action execution pipelineTable- Added public methodscreateView(),dropView(),createTrigger(),dropTrigger()for fluent API usageBaseMigration- Added convenience methods for easy migration usage without needing to manage Table objectsDatabase Adapters (all 4 adapters updated):
MysqlAdapter- MySQL-specific SQL generation withOR REPLACEsupport for viewsPostgresAdapter- PostgreSQL implementation with materialized views andEXECUTE FUNCTIONsyntax for triggersSqliteAdapter- SQLite-specific syntax withBEGIN...ENDblocks for triggersSqlserverAdapter- SQL Server implementation withIF OBJECT_IDchecksUsage Examples
Basic View
Materialized View (PostgreSQL)
Triggers
Using Table API
Database-Specific Notes
MySQL/MariaDB
OR REPLACEsyntax viareplaceoptionFOR EACH ROW(statement-level not supported)ORoperatorPostgreSQL
OR REPLACEsyntax (except materialized views)materializedoptionEXECUTE FUNCTIONsyntaxSQLite
IF NOT EXISTS(no replace support)BEGIN...ENDblock around definitionSQL Server
IF OBJECT_IDcheck for replace functionalityASkeyword instead ofBEGINORTesting
Added comprehensive test coverage in
MysqlAdapterTest:testCreateView()- Verifies view creation and queryingtestDropView()- Verifies view removaltestCreateTrigger()- Verifies trigger creation and executiontestDropTrigger()- Verifies trigger removalAll tests pass code style checks (PHPCS) and syntax validation.
Documentation
Created
docs/examples/ViewsAndTriggersExample.phpwith:Code Quality
Breaking Changes
None. This is a purely additive change with no modifications to existing behavior.
Related Issues
Closes #347
Questions for Reviewers