Fix unwanted SQLite schema emulation in SqliteSchemaManager

[mvorisek]

Q	A
Type	bug
Fixed issues	#6314

Summary

Missed in #4804 and fix related #5517.

When merging up (into 4.x), simply remove the str_replace.

[derrabus]

Can we cover this with a test please?

[mvorisek]

Is your request for a one functional test into Doctrine\DBAL\Tests\Functional\Schema\SqliteSchemaManagerTest?

[mvorisek]

@derrabus may I understand your wishes better?

[derrabus]

A test please.

[mvorisek]

@derrabus may I ask you for a review?

[derrabus]

Sure. It's on my todo list, but I don't have much time to spend on open source work at the moment.

[derrabus]

I don't quite understand this test, tbh. You're testing with a dot in a table name that SQLite to my astonishment seems to accept when quoted. But that doesn't have much to do with schema emulation. Shouldn't the test work with a table from an actual secondary schema?

[mvorisek]

The fixed methods accept database name using 1st parameter, the table name is accepted as 2nd parameter, thus the 2nd parameter can be unquoted and dot must be preserved (it cannot contain schema).

As the fixed methods cannot be reached easily from public API as table with schema and dot is not fully supported yet, the test is coded to call the fixed protected methods.

[derrabus]

As the fixed methods cannot be reached easily from public API as table with schema and dot is not fully supported yet, the test is coded to call the fixed protected methods.

So, what does this PR fix then? An implementation detail?

[mvorisek]

The schema manager can be extended by user classes and the protected methods must behave as expected. This PR fixes the behaviour and it is tested with functional test. For more details, see the #4804 and #5517 PRs, ie. the schema emulation is deprecated for 3.x and removed in 4.x. Can we agree on this? Is there anything else I should do?

[mvorisek]

Hi @derrabus, we are waiting for your approval. The "SQLite schema emulation to manager" was added in #5268 without any explanation. I belive it is there for historical reasons and such emulation was officially removed in #4804 (for v4.x) and in #5517 (for v3.x with the possibility to disable it).

Given these facts, I belive the emulation simply should not be there, ie. it should be disableable for v3.x and removed for v4.x.

Tests are testing the desired behaviour.

My question to you is, what can I do more to get this PR merged and if we are on the same page with the reasoning why it should be removed. Thank you!

[mvorisek]

Can this PR be please either approved or can I get an actionable feedback?

[mvorisek]

Can this PR be merged?

[derrabus]

I'm working on my DBAL backlog as I find the time to do so. I was able to clear some items from that list today and will come back to this PR soon. Bear with me.

[mvorisek]

This PR is about 2 month since approved... Can it be merged now, please.

[derrabus]

I'm sorry, this took me so long. But my concern remains: your test runs assertions on implementation details. It looks complicated and I don't want to maintain it. Your change looks like you're fixing a valid problem, but I need a functional test (no reflection voodoo) that reproduces your problem as you encounter it.

[mvorisek]

Thank you - please tell me how to test better.

[derrabus]

Write a test that reproduces your problem as you encounter it. That's all I can tell you because I have no clue why you're hitting this issue at all.

[mvorisek]

We override non-final SqliteSchemaManager class. I propose to merge this as is as in v4.0 the schema emulation has been removed and thus I would be fine with empty merge/test when merging up.

[derrabus]

We override non-final SqliteSchemaManager class.

So, do the same in the test.

The base branch was changed.

[mvorisek]

@derrabus the test was adjusted, can I have your final review?

[derrabus]

I'm touring conferences this week. Please ping me again if I haven't reviewed the PR by the end of next week.

[mvorisek]

@derrabus ping as requested