Doctrinebundle: Rename "BlacklistSchemaAssetFilter" to match "WellKnownSchemaFilterPass"

[albe]

It just makes more sense. I suggest using "UnknownSchemaAssetFilter" instead - or probably even better "WellKnownSchemaAssetFilter", since it actually filters out well known assets. In case the filter itself should be more generic, "ExcludedSchemaAssetFilter" could work well, too.

TBD: Only change this in master or target lower. AFAIS it would only be breaking for 3rd parties that actually extend BlacklistSchemaAssetFilter or directly use it (could possibly be leviated by keeping an alias class around), but then this is a "glue" bundle, not a framework, so is this even API?

[greg0ire]

The list is passed dynamically rather than being harcoded, so the implementation could be used for something else entirely, which would make me lean towards ExcludedSchemaAssetFilter… but it does not convey the meaning as well as the original term IMO. How about ExcludeListSchemaAssetFilter?

Only change this in master or target lower. AFAIS it would only be breaking for 3rd parties that actually extend BlacklistSchemaAssetFilter or directly use it (could possibly be leviated by keeping an alias class around), but then this is a "glue" bundle, not a framework, so is this even API?

It should probably have been marked as @internal or @final, but I think a BC promise is supposed to apply now. The class is small enough, I would use copy/paste and deprecate the old class. aliases are a can of worms, and make sense when you do that kind of thing a lot of times in a project (like if you deprecate an entire namespace). I would target 2.1.x

[ostrolucky]

I am not sure if this feature is even used, I don't see this connected to anything. This comment kinda confirms it https://github.com/symfony/symfony/pull/36655#issuecomment-622959966. @weaverryan do you know about anything?

[greg0ire]

It was introduced in https://github.com/doctrine/DoctrineBundle/pull/1037 , to prevent tables that are not part of the ORM schema but rather related to various Symfony components to be dropped during a migration.

[ostrolucky]

Alright, it's needed then. But I actually like current name. As @greg0ire mentioned already, it doesn't filter out well known stuff, it's pretty generic and can be used for anything. Suggested alternative ExcludeListSchemaAssetFilter is just awkward. So I would say no to this proposal.

[greg0ire]

But I actually like current name.

You might not have seen this tweet that shows that some other people don't like it at all, probably because it strongly connects the color "black" with exclusion, which, when you think of it, doesn't make much sense. Maybe we can find a word that makes everyone happier _and_ makes things clearer in the process.

[albe]

lean towards ExcludedSchemaAssetFilter… but it does not convey the meaning as well as the original term IMO. How about ExcludeListSchemaAssetFilter?

Hmm. Why does it not convey the meaning as well for you? Is it due to the active terminology? ExcludeSchemaAssetFilter could work in that case.

Looking at what it does, I would phrase it something like this: "A filter that skips specific assets for a given schema". SkipSchemaAssetFilter?

I mean, that it uses a "blacklist" is rather a technical detail even.

[dkarlovi]

Ignored, since it's literally ignored, not taken into account?

[albe]

Ignore(d)SchemaAssetFilter - well, would work for me - I'm personally leaning towards the active form.

[greg0ire]

Why does it not convey the meaning as well for you?

When I see Exclude(d)SchemaAssetFilter, I think that the it's an asset filter based on an exclude schema, whatever that is, while you group asset with schema rather than filter (it's a filter that works with excluded schema assets).

Note that in the DBAL, which is where this is used in the end, it's called a schema assets filter, with an s: https://github.com/doctrine/dbal/search?q=getSchemaAssetsFilter&type=Commits, in the end, it is used in array_filter: https://github.com/doctrine/dbal/blob/4c03ed81471c62f178581eb325339a3c34f3b71a/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php#L235

I mean, that it uses a "blacklist" is rather a technical detail even.

Yeah but that's what differentiates it from https://github.com/doctrine/DoctrineBundle/blob/1.12.x/Dbal/RegexSchemaAssetFilter.php : how it is implemented technically.

So to me, it could {Ignore(d)List,Exclude(d)List}SchemaAssetsFilter (note the s), but you have to somehow realise that internally, it relies on a list of things to remove.

[albe]

Yeah but that's what differentiates it from https://github.com/doctrine/DoctrineBundle/blob/1.12.x/Dbal/RegexSchemaAssetFilter.php : how it is implemented technically.

Good point. Thanks for explaining.

So to me, it could {Ignore(d)List,Exclude(d)List}SchemaAssetsFilter (note the s)

Sounds reasonable and in that case I'd personally prefer IgnoreListSchemaAsset(s)Filter - with the added s the RegexSchemaAssetFilter needs to be changed too probably to keep things in line.

[nicolas-grekas]

Shouldn't we just deprecate this class? I'm not sure anybody uses it anymore.

[greg0ire]

@nicolas-grekas can you please elaborate? To me, it looks like it's still very much in use: https://github.com/doctrine/DoctrineBundle/blob/ffea292dbf152d124e7a188303c8ec7ac9d3ae4c/DependencyInjection/Compiler/WellKnownSchemaFilterPass.php#L54

and

https://github.com/doctrine/DoctrineBundle/blob/f5a5a3dd1351818080b609b544608b472b07029f/DoctrineBundle.php#L41

But since you are the author, you probably have things in mind I don't.

[nicolas-grekas]

I think that after https://github.com/doctrine/DoctrineBundle/pull/1163, Symfony is not going to need that pass+denylist anymore.

[greg0ire]

It does seem to cover the messenger and cache table case, but there are also tables for storing session and information related to the Lock component: https://github.com/doctrine/DoctrineBundle/commit/e4ca3d2c7f5f9076b228d0762ee48ae2b1090340#diff-5bacf947dd7344319148e8a87bb79bcdR9-R10

[weaverryan]

Hmm, slight preference to keep the class (rename it) and deprecate it later. We really DO need to finish the work so that the session table (especially that one) is handled automatically in Symfony, so that we can then deprecate this.

[ostrolucky]

I don't see the point to rename it only so we remove it later. We also don't need to wait for Symfony to update first, we can deprecate it before that. That would only encourage dropping usages sooner.