-
Notifications
You must be signed in to change notification settings - Fork 152
Declarative schema database compatibility #264
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
Declarative schema database compatibility #264
Conversation
|
||
Introduce concept of database adapters to allow normalize values in declarative schema for different databases. | ||
|
||
`\Magento\Framework\Setup\Declaration\Schema\Db\SchemaBuilder` should accept `\Schema\Db\MySQL\DbSchemaReaderFactory` that would allow to create database specific implementations of `DbSchemaReaderInterface` for given connection. `\Schema\Db\MySQL\DbSchemaReaderFactory` will depend on `\Magento\Framework\App\DeploymentConfig` to get configuration for connection, `model` will contain name of database adapter. |
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.
Can you change \Schema\Db\MySQL\DbSchemaReaderFactory to \Magento\Framework\Setup\Declaration\Schema\Db\DbSchemaReaderFactory
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.
Resolved, it was supposed to be like you proposed :-)
Before supporting more storage engines, I suggest first fixing the existing bugs and adding missing features in the current implementation.
All of these will influence the implementation of new backends (as well as the design of the new interfaces), so it would make sense to first get one working fully, so the tests could then be run against any new storage implementations. |
|
const normal = 'normal'; | ||
|
||
// For MySQL and MS SQL fulltext, one of text indexes for Oracle and text for Postgres | ||
const text = 'text'; |
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.
Better to name it fulltext
``` | ||
|
||
``` | ||
class OnDelete extends \SplEnum |
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.
Won't these enums be considered APIs int the future? It will be hard to add new values after that. Perhaps we should just have arrays of strings set with DI configuration instead of static enums
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.
Enums allow to restrict possible values, it helps to make sure that all values supported by all databases.
|
||
## Design | ||
|
||
Introduce new interfaces under Db namespace that allow to allow get information about table schema, refactor implementations of `\Magento\Framework\Setup\Declaration\Schema\Db\DbSchemaReaderInterface` in declarative schema to use interfaces under Db namespace. |
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.
Introduce new interfaces under Db namespace that allow to allow get information about table schema, refactor implementations of `\Magento\Framework\Setup\Declaration\Schema\Db\DbSchemaReaderInterface` in declarative schema to use interfaces under Db namespace. | |
Introduce new interfaces under Db namespace that allow to get information about table schema, refactor implementations of `\Magento\Framework\Setup\Declaration\Schema\Db\DbSchemaReaderInterface` in declarative schema to use interfaces under Db namespace. |
Since this was closed without being merged, do you have a link to an architecture meeting or maybe notes stating the reasons? |
Closing it temporary, as it is not being worked on currently. Here is proposal that contains solution for the immediate problem (declarative schema compatibility with MariaDB 10.4) #318. |
Problem
Solution
Requested Reviewers