Skip to content

/ EntityAuditBundle

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

Jump to bottom

[RFC] `global_ignore_columns` param should be renamed to `global_ignore_fields` #329

Open
phansys opened this issue Jul 6, 2018 · 7 comments
Open

[RFC] `global_ignore_columns` param should be renamed to `global_ignore_fields` #329

phansys opened this issue Jul 6, 2018 · 7 comments

Comments

@phansys
Copy link

@phansys phansys commented Jul 6, 2018 •
edited

Q A
Bug report? yes
Feature request? no
BC Break report? no
Used Version 1.0.9

global_ignore_columns param must be renamed to global_ignore_fields, since its values are checked against field names instead column names.
I think we could deprecate it, explaining the new one must be used.
@phansys phansys changed the title [RFC`global_ignore_columns` param should be renamed to `global_ignore_fields` [RFC] `global_ignore_columns` param should be renamed to `global_ignore_fields` Jul 6, 2018
@tolry
Copy link
Member

@tolry tolry commented Jul 9, 2018

IIRC both configuration names are working currently, leaving the old (columns) only for compat reasons, but I might be confusing this with another ignore-config variable.
@tolry
Copy link
Member

@tolry tolry commented Jul 9, 2018

found the PR #213
@phansys
Copy link
Author

@phansys phansys commented Jul 10, 2018

In latest 1.x tag (currently v1.0.9), there is just global_ignore_columns config option:

EntityAuditBundle/src/SimpleThings/EntityAudit/DependencyInjection/Configuration.php

Lines 18 to 20 in 5725eca

->arrayNode('global_ignore_columns')
->prototype('scalar')->end()
->end()

#213 was made against master, even, that option is currently not part of that branch (2df48c3#diff-7005d8857f97a86a6140690466b7723f).
@tolry
Copy link
Member

@tolry tolry commented Jul 10, 2018

Hmm, I could have sworn, that PR was older than the 1.x-branch, sorry about the confusion. I guess it's time to decide about master branch being reverted to/merged with 1.x to avoid a complete halt in development.

Last I heard from @DavidBadura it sounded, like he wouldn't finish the master refactoring if at all - any changes there @DavidBadura? Is the lib + bundle split still happening or should we abandon that?
@phansys
Copy link
Author

@phansys phansys commented Jul 10, 2018

#213 was part of 1.x branch (1d06f6f). It is present at v1.0.1, but it was completelly removed at v1.0.3: (v1.0.1...v1.0.3).
@tolry
Copy link
Member

@tolry tolry commented Jul 10, 2018

weird, probably by mistake
@phansys
Copy link
Author

@phansys phansys commented Jul 10, 2018

Can I help? Please, don't hesitate to share your concerns.
Thank you in advance!
@phansys phansys mentioned this issue Jul 12, 2020
Merged
@phansys phansys mentioned this issue Oct 13, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
@tolry @phansys
You can’t perform that action at this time.