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

Issue #3605: enable certain SAXParserFactory features. #3614

Merged
merged 1 commit into from Dec 6, 2016
Merged

Issue #3605: enable certain SAXParserFactory features. #3614

merged 1 commit into from Dec 6, 2016

Conversation

liutikas
Copy link

@liutikas liutikas commented Dec 6, 2016

Issue #3605: enable certain SAXParserFactory features.

@codecov-io
Copy link

codecov-io commented Dec 6, 2016

Current coverage is 99.59% (diff: 100%)

Merging #3614 into master will increase coverage by <.01%

@@             master      #3614   diff @@
==========================================
  Files           275        275          
  Lines         13310      13312     +2   
  Methods           0          0          
  Messages          0          0          
  Branches       3034       3034          
==========================================
+ Hits          13256      13258     +2   
  Misses           53         53          
  Partials          1          1          

Powered by Codecov. Last update 62e1209...1350b30

@rnveach
Copy link
Member

rnveach commented Dec 6, 2016

@liutikas CI is failing because of improper commit message by our rules.
Please see https://travis-ci.org/checkstyle/checkstyle/jobs/181539876#L828-L837 and correct according.

Also please squash all commits and keep as 1.

@liutikas
Copy link
Author

liutikas commented Dec 6, 2016

@rnveach I updated the commit message and squashed the commits.

I looked at "IDEA Inspections Pull Request" check and it seems that I did not add any new warnings. I am not sure why it is failing.

@rnveach
Copy link
Member

rnveach commented Dec 6, 2016

@romani It looks like IDEA updated to newest IntelliJ version. 1255 errors, 2381 total.
@liutikas Please ignore IDEA as I believe this is unrelated to your change.

@romani
Copy link
Member

romani commented Dec 6, 2016

@liutikas , IDEA could be ignored by you in this PR.
Please explain in issue a reason of second feature that you enabled. We need more details as we can not completely recheck your requests so we need all possible details to explain this code. There might appear some conflict of interests in future, so details are required.

@liutikas
Copy link
Author

liutikas commented Dec 6, 2016

Sure thing, both features load-external-dtd and external-general-entities are normally default set to true. Our company has set their java defaults to false as it can lead to cross scripting attacks if not handled correctly. load-external-dtd feature allows to load external DTD into a an XML document and external-general-entities feature allows to these these external DTDs in elements. Using these two we are able to compose configuration files from multiple XML files. Since both of these features are enabled by default in default java set ups it should be a no-op for most checkstyle users.

@romani
Copy link
Member

romani commented Dec 6, 2016

reason was moved to issue.

@romani romani merged commit 636ae05 into checkstyle:master Dec 6, 2016
@liutikas liutikas deleted the xmlfeatures branch December 6, 2016 23:51
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.

None yet

4 participants