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
Rename "BlacklistSchemaAssetFilter" to match "WellKnownSchemaFilterPass" #1176
Comments
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
It should probably have been marked as |
I am not sure if this feature is even used, I don't see this connected to anything. This comment kinda confirms it symfony/symfony#36655 (comment). @weaverryan do you know about anything? |
It was introduced in #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. |
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 |
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. |
Hmm. Why does it not convey the meaning as well for you? Is it due to the active terminology? Looking at what it does, I would phrase it something like this: "A filter that skips specific assets for a given schema". I mean, that it uses a "blacklist" is rather a technical detail even. |
Ignored, since it's literally ignored, not taken into account? |
|
When I see 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
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 |
Good point. Thanks for explaining.
Sounds reasonable and in that case I'd personally prefer |
Shouldn't we just deprecate this class? I'm not sure anybody uses it anymore. |
@nicolas-grekas can you please elaborate? To me, it looks like it's still very much in use:
and DoctrineBundle/DoctrineBundle.php Line 41 in f5a5a3d
But since you are the author, you probably have things in mind I don't. |
I think that after #1163, Symfony is not going to need that pass+denylist anymore. |
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: e4ca3d2#diff-5bacf947dd7344319148e8a87bb79bcdR9-R10 |
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. |
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. |
#1176 Deprecate (Blacklist|WellKnown) schema filters
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?The text was updated successfully, but these errors were encountered: