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

#280 Removal of hardcoded revision field name #281

Merged

Conversation

c0ntax
Copy link
Contributor

@c0ntax c0ntax commented May 8, 2017

No description provided.

@smoench smoench requested review from andrewtch and removed request for andrewtch May 8, 2017 11:54
Copy link
Contributor

@tolry tolry left a comment

Choose a reason for hiding this comment

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

lgtm 👍

Is security an issue here? Column names are hard to escape (DBMS independent) and this would make it possible to inject SQL via config. imho not a real issue, since config is mostly maintained by people already having direct db access.

.gitignore Outdated
@@ -4,3 +4,4 @@ vendor
tests/SimpleThings/Tests/EntityAudit/cache
tests/SimpleThings/Tests/EntityAudit/Proxies
.php_cs.cache
.idea
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert - this is not a project specific ignore pattern and should be maintained in your global gitignore (system-, ide-dependent ignores)

@tolry
Copy link
Contributor

tolry commented May 9, 2017

any objections: @DavidBadura @andrewtch @smoench ? I am really not familiar with the code base, but cannot imagine any problems with these changes. If noone objects, I will merge this.

@DavidBadura do you need to cherry pick this for master?

@DavidBadura
Copy link
Contributor

LGTM 👍

@tolry i will release a new 1.0.x version and merge this into master soon.

@c0ntax thank you!

@DavidBadura DavidBadura merged commit a731857 into sonata-project:1.0 May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants