intellij-community icon indicating copy to clipboard operation
intellij-community copied to clipboard

Fix #2 IDEA-141538: XML validator does not use a relative path to the XSD

Open rostidev opened this issue 6 years ago • 5 comments

This is a continue of my previous already closed PR: https://github.com/JetBrains/intellij-community/pull/1014 (Fix IDEA-141538: XML validator does not use a relative path to the XSD) that was partially accepted.

Please assign to @dmitry-avdeev

rostidev avatar Dec 22 '18 19:12 rostidev

This PR contains both the original https://github.com/JetBrains/intellij-community/pull/1014 and the test fixing commits. My previous PR was accepted only partially because of those tests failure. When you merge this new PR accept its versions of files or revert that partial commit before merging this new PR.

rostidev avatar Dec 22 '18 19:12 rostidev

@rosti-il do we really need this pull request when we have commit with your partial changes of previous pull request https://github.com/JetBrains/intellij-community/commit/b943c394f8392da17b32e12a42f2e73fb98aea19

nicity avatar Dec 22 '18 21:12 nicity

@nicity I think you do.

  1. In the current code at master branch you still register two instances of two different EntityResolver implementations three times!
  2. The third registration is done by a dirty way within the startDocument() method of DefaultHandler when parsing already running
  3. Call to the configureEntityManager() is also done by a dirty way within the startDocument() method.

Also, the new code is cleaner. I'm also not sure the current code at master resolves entity links like file:./some.dtd always right. This is one of the cases of fixed test fails in XmlHighlightingTest. The if (publicIdUri.isOpaque()) check handles such entity links.

Is there any reason not to merge it just because I forgot to run tests before submitting the first PR and @dmitry-avdeev took that PR partially? Please read what he wrote me at https://github.com/JetBrains/intellij-community/pull/1014#issuecomment-448944307

rostidev avatar Dec 22 '18 22:12 rostidev

This version breaks XmlEntityManagerCachingTest (we do need to call configureEntityManager() in startDocument() to cache the parsed entities). Also, please resolve conflicts with current master branch.

dmitry-avdeev avatar Dec 27 '18 17:12 dmitry-avdeev

@dmitry-avdeev, interesting. If I select all packages within the intellij.xml.tests module they all pass but if I select only the com.intellij.xml package within that module there are a few tests that fail, including XmlEntityManagerCachingTest. I will fix them.

Isn't there some IntelliJ bug in running UnitTests?

rostidev avatar Dec 27 '18 21:12 rostidev