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

Disable escaping of non-latin characters #52

Closed
wants to merge 2 commits into from

Conversation

Ovsyanka
Copy link
Contributor

@Ovsyanka Ovsyanka commented May 14, 2018

This PR mainly solves #21.

I want to point to a few aspects of my changes:

  1. As you can see, I removed
$document->formatOutput = true;
$document->normalizeDocument();

Because tests not failed and I don't see why it needed. I could return it add appropriate tests if you explain me reason of this code.
2. I removed a lot of checks instanceof DOMDocument because in fact there is always DOMDocument in the $node in case if canonicalize is always true.
3. I removed $canonicalize because in fact it is always true and the unused argument just make the code maintenance harder. Especially without corresponding unit-tests.
4. I made nodeToText public to be able to test it.
5. I see the possible error with the $ignoreCase argument (it ignores case when set to false) and I could fix it in this PR or in the different. What do you think? Fixed in the #53

I'll be happy to receive any criticism/suggestions about the appearance of my changes and tests.

@codecov-io
Copy link

codecov-io commented May 14, 2018

Codecov Report

Merging #52 into master will increase coverage by 3.12%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #52      +/-   ##
============================================
+ Coverage     79.14%   82.26%   +3.12%     
+ Complexity      122      121       -1     
============================================
  Files            15       15              
  Lines           326      327       +1     
============================================
+ Hits            258      269      +11     
+ Misses           68       58      -10
Impacted Files Coverage Δ Complexity Δ
src/DOMNodeComparator.php 100% <100%> (+41.66%) 9 <4> (-1) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed5fd22...83c4463. Read the comment docs.

@sebastianbergmann
Copy link
Owner

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it as it makes many unrelated changes.

@sebastianbergmann
Copy link
Owner

FYI: * $canonicalize is not always true. This flag ic controlled by assertEquals(). If true, then the C14N() method is used to canonicalize the XML.

@sebastianbergmann
Copy link
Owner

FYI: $node is not always a DOMDocument object, the comparator can also process document fragments.

@Ovsyanka
Copy link
Contributor Author

Ovsyanka commented May 23, 2018

FYI: * $canonicalize is not always true.

Look closely at the diff. The only place where private function nodeToText() called is DOMNodeComparator::assertEquals:
$expectedAsString = $this->nodeToText($expected, true, $ignoreCase);

And there hardcoded true value for the $canonicalize argument.

FYI: $node is not always a DOMDocument object

I meant that in fact it always become DOMDocument in the first lines of nodeToText here: 83c4463#diff-0665d25cb8db14b20881e327fe1ca491L75

@Ovsyanka
Copy link
Contributor Author

However, I have decided not to merge it as it makes many unrelated changes.

I agree with you, that there not only changes to fix the issue, but some refactoring changes in addition. I could try to split refactoring and actual issue fix in different commits to more easy analysis. I'll wait for your responce thought.

@Ovsyanka
Copy link
Contributor Author

Ovsyanka commented May 23, 2018

FYI: I decided to not touch the question "should the $canonicalise value in the DOMNodeComparator::assertEquals be ignored or shouldn't". Because it is other question and I will raise it later, maybe, in diffenrent issue. I made here (intended, at least) only one funtional change. And I see, that you already merged #53 (ignore case issue) so my 5-th point doesn't actual anymore.

@sebastianbergmann
Copy link
Owner

I prefer single-purpose pull requests. Feel free to send new pull requests, one for each purpose, not mixing refactoring, new functionality, and bug fixes.

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