steveire / grantlee

Libraries for text templating with Qt
Other
141 stars 48 forks source link

Escape double-quotes as the HTML 4 specs says and use a more #8

Closed dantti closed 4 years ago

dantti commented 9 years ago

Qt QString::toHtmlEscaped() has a more efficient way of escaping but it does not escape single-quotes, which is what Django counterpart does, on the other hand Grantlee doesn't escapes double-quotes which is what HTML 4 specs says to do.

steveire commented 9 years ago

Please put relevant information in the commit message, not the pull request. Make sure you leave a blank line between the commit title and the rest of the message.

Aside from that, the "basic-syntax27" now gives a different result than the same django test. Why is that? Are we missing something?

dantti commented 9 years ago

That's because " is escaped regardless if there is a backslash before it, IMO that's the right behavior otherwise one can submit values with \" and have the the " preserved, I'd say that to conform the tests with Django the literal string on the template must be parsed differently.

dantti commented 9 years ago

Also Django does escaping on double-quotes which Grantlee before this patch didn't

steveire commented 9 years ago

Sorry, I tried to understand why the basic-syntax27 test needs to change behavior away from what it currently is, and away from how django behaves, but I don't get it.

I also spent some time trying to find the "escape01" test in the django code or git history, and I haven't found it yet. It is unlikely that I just made that test up myself, so I'll keep looking at some point later. I think it would help to understand this change.

dantti commented 4 years ago

The reason why the test has to change is because the replaces you did doesn't escape double quotes, which is crucial if you put some text into a tag attribute: <a href="{{ somevar }}">foo</a>

codecov-io commented 4 years ago

Codecov Report

Merging #8 into master will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #8      +/-   ##
==========================================
+ Coverage   89.06%   89.06%   +<.01%     
==========================================
  Files         158      158              
  Lines       11275    11276       +1     
==========================================
+ Hits        10042    10043       +1     
  Misses       1233     1233
Impacted Files Coverage Δ
templates/lib/outputstream.cpp 80% <100%> (+3.33%) :arrow_up:
templates/tests/testbuiltins.cpp 99.46% <100%> (-0.01%) :arrow_down:

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 b6454a9...a363a9f. Read the comment docs.

steveire commented 4 years ago

Sorry for the delay. I fixed up the commits as I described previously.