Skip to content
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

Deprecate SchemaDiff::toSql() and SchemaDiff::toSaveSql() #5766

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

morozov
Copy link
Member

@morozov morozov commented Oct 16, 2022

Q A
Type deprecation
  1. Similar to TableDiff::getName() (Deprecate not passing $fromColumn to TableDiff #5678), implementing the SQL-related methods makes the diff (which is a value object) depend on the infrastructure. It should be the other way around.
  2. Being able to discard all DROP statements from the generated schema migration SQL is a poor-man solution to dealing with the objects not owned by the DBAL. It has the following downsides:
    1. The DBAL still can choke during introspection of those objects (e.g. Schema introspection on Oracle is failing when user is 'system' #4470, AbstractSchemaManager#_getPortableTableColumnList() cannot process functional indexes defined in MySQL #5306).
    2. The objects that are owned by the DBAL still need to be dropped manually.
  3. The toSaveSql() method and the corresponding $saveMode parameter names make absolute no sense.
  4. Neither the DBAL nor Migrations use SchemaDiff::toSaveSql(). This method is absolutely untested, and I'm not talking about a unit test that confirms that the flag is respected. I'm talking about the integration testing of the scenarios for which the method was implemented in the first place.

Instead of using SchemaDiff::toSaveSql() for discarding schema changes in 3rd-party database objects (if that's the case), it is recommended to use asset filtering.

@morozov morozov marked this pull request as ready for review October 16, 2022 15:15
@morozov morozov added this to the 3.5.0 milestone Oct 16, 2022
@derrabus
Copy link
Member

Shall we add tests that call the new getAlterSchemaSQL() method?

@morozov morozov force-pushed the deprecate-schema-diff-save-mode branch from 37f683f to 5bbcdf1 Compare October 16, 2022 16:01
@morozov
Copy link
Member Author

morozov commented Oct 16, 2022

We can migrate more code to use the new API. It will make both the new and the deprecated API covered by existing tests.

@morozov morozov merged commit 4873e3a into doctrine:3.5.x Oct 17, 2022
@morozov morozov deleted the deprecate-schema-diff-save-mode branch October 17, 2022 14:45
@greg0ire
Copy link
Member

Neither the ORM nor Migrations use SchemaDiff::toSaveSql()

That does not seem to be accurate: https://github.com/doctrine/orm/blob/5f4b76b88fe2682af2c1d7580719dd1b9193db65/lib/Doctrine/ORM/Tools/SchemaTool.php#L950

What should be done here?

@morozov
Copy link
Member Author

morozov commented Oct 18, 2022

Sorry, I meant DBAL instead of ORM. I knew that the ORM uses it. The ORM should deprecate not passing the --complete flag to the schema tool. This flag will be not supported with DBAL 4.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants