-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support for extra SQL statements in table creation and dropping #7261
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
Closed
+296
−0
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
I don't think we should be adding new options to the Table class. Options is an untyped associative array whose value types need to be validated at runtime. Using options has been already deprecated for some schema objects like indexes.
And as for the feature itself, Table is a value object that represents the table schema. The SQL used to create the table is defined by the platform, not Table. If we allow that, I can imagine all sorts of issues with schema introspection, migrations and so on.
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.
I'm not sure I fully get the concern but anyway: would you like to suggest me an alternative that'd address your requirements and that'd enable the target I'm trying to achieve here?
Uh oh!
There was an error while loading. Please reload this page.
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.
No, sorry. I don't know how exactly the schema management capabilities from DBAL are integrated into Symfony and what the use case is.
Uh oh!
There was an error while loading. Please reload this page.
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.
In Symfony, this is used to generate migration files that contain extra statements, in our case a trigger and a function on PgSQL, to leverage
pg_notify. See symfony/symfony#62820 for how this PR would be used. That's quite simple and effective plumbing.Uh oh!
There was an error while loading. Please reload this page.
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.
It’s also a footgun similar to custom column DDL where people use it for various workarounds and then get surprised why their schema migrations don’t work as expected.
In this specific case, it doesn’t look like you need the DBAL to execute these platform-specific and application-specific statements. You can execute them in Symfony before or after the migration.
Uh oh!
There was an error while loading. Please reload this page.
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.
I'm not sure I follow your lead here. Symfony isn't really involved here, this is all doctrine/* code, with a lot of hardcoded stuff. About the risk either: I don't see how preventing ppl from achieving what they need by closing everything makes a better platform.
What we need to achieve is generating migration files with extra statements that people can then inspect/commit. How does hooking before/after the migration come into play in your mind?