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

Implement {Configuration#getSchemaAssetsFilter} #3308

Merged

Conversation

bezhermoso
Copy link
Contributor

@bezhermoso bezhermoso commented Oct 4, 2018

Q A
Type feature
BC Break no
Fixed issues #3196

Summary

Gives ability to specify a callable to use for filtering schemas by potentially something other than regular expressions. Implements what's laid out by @morozov in his comment: #3196 (comment)

  • Feature implementation
  • New tests for the feature
  • Pass existing tests

@bezhermoso bezhermoso changed the title Implement {Configuration#getFilterSchemaAssetsCallable} Implement {Configuration#getSchemaAssetsFilter} Oct 4, 2018
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bezhermoso, the patch looks good overall. Please see a few comments.

lib/Doctrine/DBAL/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Configuration.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php Outdated Show resolved Hide resolved
@bezhermoso
Copy link
Contributor Author

@morozov, I think I addressed your comments. Let me know if I miss anything.

@bezhermoso bezhermoso force-pushed the custom-asset-filter-as-callable branch from ae88543 to 348e600 Compare October 6, 2018 04:14
@bezhermoso bezhermoso force-pushed the custom-asset-filter-as-callable branch from 348e600 to 5110a19 Compare October 6, 2018 04:36
Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🚢

@morozov morozov merged commit 18a8040 into doctrine:master Oct 6, 2018
@morozov
Copy link
Member

morozov commented Oct 6, 2018

Thank you, @bezhermoso.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants