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

Address dbal deprecations #10153

Merged
merged 2 commits into from
Oct 24, 2022
Merged

Conversation

greg0ire
Copy link
Member

No description provided.

@greg0ire greg0ire force-pushed the address-dbal-deprecations branch 4 times, most recently from e682164 to b05227e Compare October 18, 2022 21:40
@greg0ire
Copy link
Member Author

@morozov the failing test calls the update command, then feeds its output to the DBAL. It seems that it is choking on DROP TABLE quote-address;, and the weird thing is that I got that information from the DBAL 2 job. The DBAL 3 jobs don't display that information, is that expected?
Regarding that -, did this test just discover a latent bug?

@morozov
Copy link
Member

morozov commented Oct 18, 2022

Regarding that -, did this test just discover a latent bug?

There are two issues with this potentially:

  1. The UpdateCommandTest isn't isolated. It seems to drop whatever schema that the previous tests created. If you run it alone, it will pass. Prior to this change, the DBAL would omit all DROP statements, so the test would pass.
  2. When accepting the $table parameter as a string AbstractPlatform::getDropTableSQL() expects it to be an SQL fragment (not just a value representing name), so the caller is supposed to quote the name. For some reason the $table with the name quote-address has $quoted = false in its properties which leads to the unquoted name being passed to the method. UPD: the problem seems to be in the DBAL which is what introspects the schema. The object names are automatically marked as quoted only by the Oracle and DB2 schema manager while certain quoting rules apply to all platforms.

The DBAL 3 jobs don't display that information, is that expected?

No idea.

@morozov
Copy link
Member

morozov commented Oct 18, 2022

I filed doctrine/dbal#5776 for documentation purposes but it's one of the side effects of doctrine/dbal#4357, so it won't be fixed in the near future.

@greg0ire
Copy link
Member Author

Thanks for looking into this! Do you think it will be fixed in the far future? Because I pushed something that should fix the issue until then.

@derrabus the fix implies using stderr, if you think it is right, I can review other Symfony commands to make them unix filters as well

@greg0ire greg0ire force-pushed the address-dbal-deprecations branch 2 times, most recently from 6fbe0cf to 78185f2 Compare October 19, 2022 06:45
@morozov
Copy link
Member

morozov commented Oct 19, 2022

Do you think it will be fixed in the far future?

It can be fixed even in the near future but I don't plan for it at the moment. If anybody is interested in fixing it themselves sooner, I can facilitate the process.

@greg0ire
Copy link
Member Author

Blocked by #10162

The new method AbstractPlatform::getAlterSchemaSQL() should be preferred
when available.
@greg0ire greg0ire force-pushed the address-dbal-deprecations branch 4 times, most recently from 1e090fb to f599d5a Compare October 23, 2022 21:14
Comment on lines 980 to 982
if (! method_exists($schemaDiff, 'toSaveSql')) {
throw GlobalNotSupported::createForDbal4('Using save mode when getting update schema SQL');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like dead code to me. We don't support DBAL 4 on this branch and on the 3.0.x branch, we're going to remove the $saveMode parameter anyway.

This implies deprecating a feature relying on that method.
@greg0ire greg0ire marked this pull request as ready for review October 23, 2022 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants