[PATCH] lib: remove ineffective html_escape implementation, use escape instead

[Mads Kiilerich]

On 04/13/2015 07:00 PM, Andrew Shadura wrote:
> # HG changeset patch
> # User Andrew Shadura <andrew at shadura.me>
> # Date 1428965992 -7200
> #      Tue Apr 14 00:59:52 2015 +0200
> # Node ID abeb4a96c92a913b61e2fcb9c9c87f4d02ea00a2
> # Parent  caef25781d8cb4b9e43e0def6b7a199c3f3cb462
> lib: remove ineffective html_escape implementation, use escape instead
>
> lib.helpers.html_escape scanned the whole string replacing HTML-unsafe
> characters; webhelpers, however, use optimised implementation from markupsafe.

I agree that html_escape seems weird and inefficient - it would be 
better to reduce complexity and consistently use the existing escape web 
helper.

html_escape is however only used for tests. It might make sense to have 
a simple and 'obviously correct' html escaper for use in the tests so we 
don't "beg the question". We should thus perhaps move the function to 
the tests somewhere. Or perhaps just hardcode the expected strings so we 
get simpler tests that don't do any computation. The pytest experts can 
perhaps give some advice on the right way to do that.

> Also, formencode uses its own implementation, html_quote, which is used in
> form validators. For uniformity, patch it to use escape function from webhelpers.

This seems like an unrelated change?

Formencode is weird ... but it seems like it works correctly even though 
it does weird things?

I don't like that it adds extra complexity and risk by hacking module 
externals (apparently) without achieving anything important. I would 
prefer to leave this part out.

/Mads

>
> diff --git a/kallithea/lib/compat.py b/kallithea/lib/compat.py
> --- a/kallithea/lib/compat.py
> +++ b/kallithea/lib/compat.py
> @@ -566,3 +566,7 @@ else:
>               memo[id(self)] = result
>               result.__init__(deepcopy(tuple(self), memo))
>               return result
> +
> +import formencode.rewritingparser
> +import webhelpers.html
> +formencode.rewritingparser.html_quote = webhelpers.html.escape
> diff --git a/kallithea/lib/helpers.py b/kallithea/lib/helpers.py
> --- a/kallithea/lib/helpers.py
> +++ b/kallithea/lib/helpers.py
> @@ -89,19 +89,6 @@ def canonical_hostname():
>           parts = url('home', qualified=True).split('://', 1)
>           return parts[1].split('/', 1)[0]
>   
> -def html_escape(text, html_escape_table=None):
> -    """Produce entities within text."""
> -    if not html_escape_table:
> -        html_escape_table = {
> -            "&": "&",
> -            '"': """,
> -            "'": "'",
> -            ">": ">",
> -            "<": "<",
> -        }
> -    return "".join(html_escape_table.get(c, c) for c in text)
> -
> -
>   def shorter(text, size=20):
>       postfix = '...'
>       if len(text) > size:
> diff --git a/kallithea/tests/functional/test_admin_users.py b/kallithea/tests/functional/test_admin_users.py
> --- a/kallithea/tests/functional/test_admin_users.py
> +++ b/kallithea/tests/functional/test_admin_users.py
> @@ -94,7 +94,7 @@ class TestAdminUsersController(TestContr
>                                                  '_authentication_token': self.authentication_token()})
>   
>           msg = validators.ValidUsername(False, {})._messages['system_invalid_username']
> -        msg = h.html_escape(msg % {'username': 'new_user'})
> +        msg = h.escape(msg % {'username': 'new_user'})
>           response.mustcontain("""<span class="error-message">%s</span>""" % msg)
>           response.mustcontain("""<span class="error-message">Please enter a value</span>""")
>           response.mustcontain("""<span class="error-message">An email address must contain a single @</span>""")
> diff --git a/kallithea/tests/functional/test_login.py b/kallithea/tests/functional/test_login.py
> --- a/kallithea/tests/functional/test_login.py
> +++ b/kallithea/tests/functional/test_login.py
> @@ -114,7 +114,7 @@ class TestLoginController(TestController
>                                                'lastname': 'test'})
>   
>           msg = validators.ValidUsername()._messages['username_exists']
> -        msg = h.html_escape(msg % {'username': uname})
> +        msg = h.escape(msg % {'username': uname})
>           response.mustcontain(msg)
>   
>       def test_register_err_same_email(self):
> @@ -179,7 +179,7 @@ class TestLoginController(TestController
>   
>           response.mustcontain('An email address must contain a single @')
>           msg = validators.ValidUsername()._messages['username_exists']
> -        msg = h.html_escape(msg % {'username': usr})
> +        msg = h.escape(msg % {'username': usr})
>           response.mustcontain(msg)
>   
>       def test_register_special_chars(self):
> @@ -240,7 +240,7 @@ class TestLoginController(TestController
>           )
>   
>           msg = validators.ValidSystemEmail()._messages['non_existing_email']
> -        msg = h.html_escape(msg % {'email': bad_email})
> +        msg = h.escape(msg % {'email': bad_email})
>           response.mustcontain()
>   
>       def test_forgot_password(self):
> diff --git a/kallithea/tests/functional/test_my_account.py b/kallithea/tests/functional/test_my_account.py
> --- a/kallithea/tests/functional/test_my_account.py
> +++ b/kallithea/tests/functional/test_my_account.py
> @@ -181,7 +181,7 @@ class TestMyAccountController(TestContro
>           from kallithea.model import validators
>           msg = validators.ValidUsername(edit=False, old_data={})\
>                   ._messages['username_exists']
> -        msg = h.html_escape(msg % {'username': 'test_admin'})
> +        msg = h.escape(msg % {'username': 'test_admin'})
>           response.mustcontain(u"%s" % msg)
>   
>       def test_my_account_api_keys(self):
> _______________________________________________
> kallithea-general mailing list
> kallithea-general at sfconservancy.org
> http://lists.sfconservancy.org/mailman/listinfo/kallithea-general