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

Ability to call setFilterSchemaAssetsExpression multiple times (or create an array of FilterSchema expressions) #3196

Closed
mmucklo opened this issue Jun 22, 2018 · 6 comments
Assignees
Labels
Hacktoberfest Good issue for participating in Hacktoberfest Improvement Schema Management
Milestone

Comments

@mmucklo
Copy link
Contributor

mmucklo commented Jun 22, 2018

Feature Request

Q A
New Feature yes
RFC yes
BC Break no

Summary

First thanks for all the hard work on this core library. It's been fantastic to have something like this to use.

I'm presently maintaining a symfony bundle that allows people to use either Doctrine ORM or ODM as their datastore, as well as other options (DtcQueueBundle). One of the requests has been if people don't use ORM, then the Doctrine Migrator shouldn't generate the tables for it See this issue.

This traces back to setFilterSchemaAssetsExpression which of course I could call in my compiler pass if ORM is not used, however it only allows a single filter at a time, so if someone sets a filter in their schema_filter symfony configuration, I can't add an additional filter on top of that to filter out the DtcQueueBundle tables without overwriting that filter.

I could write code to parse any existing filter's regular expression and then add additional regular criteria to the same original one, but that seems tricky and prone to error.

I'd rather just have the ability to "addFilterSchemaAssetsExpression" and the filter expressions be treated like an array.

Nevertheless if there's a better way to do this, I'm all ears.

@morozov
Copy link
Member

morozov commented Jun 22, 2018

Given that currently assets are filtered by a regular expression, it's indeed hard to extend. A trick with combining multiple regular expressions in one using positive lookaheads requires the pieces to not contain the delimiters and flags which is not the case here.

The ability to addFilterSchemaAssetsExpression() is also not really extensible. This way, the AND logic would be implemented in filterAssetNames() which won't allow any more complex extension (e.g. the OR logic). It would also require adding removeFilterSchemaAssetsExpression() and other related methods.

Instead, getSchemaAssetsFilter() : ?callable and setSchemaAssetsFilter(?callable $filter) could be added. This way, any component can wrap an existing filter using any logic and set it back.

@Majkl578, @Ocramius, do you have an opinion? I haven't used this API myself.

@Ocramius
Copy link
Member

I don't use this feature either, but regex-based filtering obviously reaches its limits very easily.

Please do not attempt to extend the regex support, and do not start with regex parsers.

Instead, let's see if a different API (callback-based, like @morozov proposed) can be implemented.

@mmucklo
Copy link
Contributor Author

mmucklo commented Jun 23, 2018

@morozov A callback implementation could work with as proposed. The only question is how to preserve backward compatibility with the existing regex filter.

Maybe the default callable stored could be the existing regex function (while the old method is marked as @deprecated) . That way someone could do a getSchemaAssetsFilter() and wrap even any present regex...

@morozov
Copy link
Member

morozov commented Jun 23, 2018

@mmucklo yes, we need a backward-compatible solution. To summarize your suggestion above:

  1. Setting a regex filter should make it available as a callback via the callback getter.
  2. Setting a callback filter resets the regex one.
  3. The regex approach is deprecated.

This way, if the caller doesn't set the callback filter, the regex will work. If a regex filter is set, it will work via both callback and regex APIs.

@morozov morozov added the Hacktoberfest Good issue for participating in Hacktoberfest label Oct 3, 2018
@morozov
Copy link
Member

morozov commented Nov 2, 2018

Fixed by #3308.

@morozov morozov closed this as completed Nov 2, 2018
@morozov morozov added this to the 2.9.0 milestone Nov 2, 2019
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Hacktoberfest Good issue for participating in Hacktoberfest Improvement Schema Management
Projects
None yet
Development

No branches or pull requests

3 participants