Whilelist existing assets we know about from metadata in SchemaTool::getUpdateSchemaSql()

[nicolas-grekas]

This PR is a sidekick of doctrine/DoctrineBundle#1037

When generating schema diffs, if a source table is filtered out by a schema_filter expression, then a CREATE TABLE is always generated, even if the table already exists.

This PR makes SchemaTool::getUpdateSchemaSql() whitelist tables that are listed in the metadata we are generating the diff for, thus fixing the issue.

[jwage]

I understand the problem and how it manifests itself to the end user. I've been running in to this in Symfony with the messenger tables. Kudos to you for taking a stab at fixing it. I always felt like the filter functionality was sort of half baked.

Can you think of any better way we could implement this to eliminate the nested closures?

[nicolas-grekas]

Passing $toFilter to createSchema()could be made to produce the desired behavior, or a new method dedicated to the need.

But that's an API change so it's out of reach for a bug fix on the ORM side. ORM 2.7 could bump to dbal 2.10 and clean the code afterwards (actually not because dbal 2.10 is planned to require PHP 7.2 apparently.)

[nicolas-grekas]

Now with tests, courtesy of @l-vo, big thanks to him!

[alcaeus]

Looks good to me. As for the nested closures, there isn't much we can do since DBAL only takes a single closure as a filter. If you want to expand on that filter later on, there's no other option than to wrap it. In one of our bundles we have a chain to take care of that, but DBAL itself does not provide any helpers for this purpose.

Introducing a chain filter class would require this to go into 2.7 as it's a new API, so I'm ok with keeping the nested closures and treating this as a (long-awaited) bug fix in 2.6.

[Ocramius]

LGTM - discussed directly with @nicolas-grekas, and the use-case seems clear to me now 👍

[Ocramius]

What I'm still puzzled on is BC compliance of this patch: we're now allowing the schema diffing to kick in for known symbols, which may break existing use-cases (where mappings were kept only as documentation, never really used by the ORM).

@doctrine/doctrinecore ideas?

[jwage]

It is definitely a "BC Break" but I am not sure in what ways it would manifest itself in desirable ways that someone would depend on, but who knows. In the problem Symfony team is trying to fix, the resulting SQL we produce just has to be manually edited by the developer before reviewing and executing to remove the tables that shouldn't be there.

[lcobucci]

Just some small changes, we should be good to go soon.

[lcobucci]

I'll plan this for v2.7.1 so we don't miss this. I think it might be good to mention the minor BC-break in UPGRADE.md, what do you think @Ocramius?

[nicolas-grekas]

Comments addressed @lcobucci
I'll let you deal with the UPGRADE notes if you don't mind.

[alcaeus]

@lcobucci 2.7.1 sounds good, as does mentioning this in UPGRADE.md. I've pushed a commit to do this. Thanks @nicolas-grekas!

Comments addressed

[lcobucci]

I've unified and simplified the test case too, LGTM.

Thanks @nicolas-grekas and @l-vo!

[mvorisek]

@greg0ire why is this not merged in master?

[greg0ire]

@mvorisek because the last merge up to master is a few years old.