Issue 29947567: Issue 5333 - Ensure links in metadata and in most tags can be made relative

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+93 lines, -53 lines)			Patch
M	cms/converters.py	View	1 2 3	1 chunk	+1 line, -3 lines	0 comments	Download
R	tests/expected_output/relative/de/rel_path	View	1 2	1 chunk	+30 lines, -16 lines	0 comments	Download
R	tests/expected_output/relative/en/rel_path	View	1 2	1 chunk	+30 lines, -16 lines	0 comments	Download
R	tests/test_site/pages/rel_path.html	View	1 2	1 chunk	+21 lines, -18 lines	0 comments	Download
A	tests/test_site/templates/head-test.tmpl	View	1 2	1 chunk	+11 lines, -0 lines	0 comments	Download

Messages

Total messages: 8

Expand All Messages | Collapse All Messages

rhowell

Hi Vasily and Winsley! Hopefully this patch will help 5333 work properly. Let me know ...

Nov. 20, 2018, 4:27 a.m. (2018-11-20 04:27:42 UTC) #2

Vasily Kuznetsov

Hi Rosie! Good catch on the absence of link processing in the head and the ...

Nov. 20, 2018, 7:35 p.m. (2018-11-20 19:35:45 UTC) #3

Hi Rosie!

Good catch on the absence of link processing in the head and the fact that link
processing of the body is done twice: by the page converter and by the template
converter (somehow I haven't fully realized this even when I was reviewing the
previous patch).

So we have two separate issues here:
1. Making sure that all links (not just the ones in <a> tags) go through
`process_link`.
2. Making sure that links in the <head> of the page also go through
`process_link`.

1 was not the case before and nobody cared. I suppose the reason for not caring
is that no localized links were ever put into the other tags so it didn't
matter. However, with relative links we need to process even non-localized
links, so now it's important that we do process all links. Your patch fixes it
for href attributes, but there's also src attribute (see [2] for the list of
tags that have it), which we should also handle. This would need to be added.

Winsley: could you please confirm that running all links of the page through
`process_link` is ok and would not break websites? You might need to involve
Julian to answer this, and that would be good.

I'm not completely sure what we should do with <base> element and its href
attribute. Having a relative URL there seems to defeat the purpose a bit, but my
guess is that the safest thing would be to process it like every other href
(your code already does it). Despite being funny this should work: HTML spec
doesn't say that the URL in <base href=...> must be absolute (see [3]) and the
instructions for calculating the base URL of a document suggest that relative
URL there would work ok (see [4]).

Then there's the 2: how do we make sure the head gets processed as well. If I
follow the code correctly (definitely wouldn't bet on it at this point) all
parts of HTML that end up in the final output are run through process_links() in
TemplateConverter (see [5]) when we are applying the template to the page.
Existing code [6] makes sure that TemplateConverter runs in a relative mode and
that should be sufficient. In fact, some of the changes in the last patch are
not even necessary [7] because get_page_params() is actually never called alone
(without its output being inserted into a template) to render any pages. If I'm
right about this, the changes to get_page_params can be removed.

However, we don't have any tests for processing of links in the <head>. We will
need to add a new template to the tests that doesn't throw the <head> away (see
[8] for an example of how to include the <head> into the template) and then use
this template in rel_path page. Then we can add a <head> to this page with some
tags that contain links (<base>, <link> and <script>) inside of it.

I think another page that produces the same kinds of links from Markdown is not
necessary for testing since all link processing happens in course of the
template conversion and doesn't depend on the converter used for the original
page.

So TLDR version of my recommendations:
- Make process_link also apply to src attribute of other tags that have it (not
only <img>).
- Remove the changes to get_page_params() introduced by the last review.
- Add a template that doesn't discard the <head> and use it in rel_path test
page.
- Don't split rel_path page into HTML and MD versions.

Sorry for a lot of text ;)
Cheers,
Vasily

[1] Tags that can have HREF attribute:
https://www.w3schools.com/tags/att_href.asp
[2] Tags that can have SRC attribute: https://www.w3schools.com/tags/att_src.asp
[3] Spec for <base> element:
https://www.w3.org/TR/html52/document-metadata.html#the-base-element
[4] Calculating "frozen base URL":
https://www.w3.org/TR/html52/document-metadata.html#set-the-frozen-base-url
[5] Everything goes through process_links():
https://hg.adblockplus.org/cms/file/9e6eab6064f0/cms/converters.py#l487
[6] TemplateConverter gets the relative parameter:
https://hg.adblockplus.org/cms/file/9e6eab6064f0/cms/utils.py#l199
[7] The change to get_page_params that is not necessary:
https://hg.adblockplus.org/cms/diff/9e6eab6064f0/cms/utils.py#l1.13
[8] Including <head> of a page in the template:
https://hg.adblockplus.org/web.adblockplus.org/file/tip/templates/minimal.tmp...

https://codereview.adblockplus.org/29947567/diff/29947568/cms/bin/generate_st...
File cms/bin/generate_static_pages.py (right):

https://codereview.adblockplus.org/29947567/diff/29947568/cms/bin/generate_st...
cms/bin/generate_static_pages.py:155: params = get_page_params(source, locale,
page, format,
I think this change is not really needed: why sweat if the results of the first
pass are thrown away anyway?

rhowell

Hey, thanks for the feedback! I implemented the suggestions, but have just a couple more ...

Nov. 21, 2018, 4:37 a.m. (2018-11-21 04:37:06 UTC) #4

Vasily Kuznetsov

Hi Rosie, Yeah, the main idea is right. Here are the answers to your questions ...

Nov. 21, 2018, 11:30 p.m. (2018-11-21 23:30:48 UTC) #5

Hi Rosie,

Yeah, the main idea is right. Here are the answers to your questions and some
ideas for how to fix things that don't come out right.

> 1. When I add the template {{ head|safe }} why does it show up in the final
> output, but {{ body|safe }} does not?

That's because you have it in the page's <head> (see below).

> 2. Why do the <body> tags show up in the final output, but the <head> tags do
> not? Is this correct behavior?

Well, this seems to be the behavior. It's not very logical, I agree, but it's
probably done so to be practical, not logical. If you have a very simple dummy
template that just includes `body`, you would like everything to be passed
through. And for `head`, the template often wants to add stuff, so the tag is
stripped. These are my guesses at least.

> 3. It seems the line numbers of the data in the <head> are not preserved.
IIRC,
> we needed to preserve line numbers for jinja2 tracebacks. Does this need to be
> addressed? If so, in this patch, or a separate issue? (See [1])

I suppose you're talking about a situation when the page itself is a .tmpl file
so it's processed by Jinja and then the template of the page is again processed
by Jinja with the text of the page passed in `head` and `body` variables. The
content of <head> will be processed by Jinja as template source only in the
first pass (of the page itself) and there the lines will be preserved. In the
pass of the template the content of <head> comes as the value of the variable so
it's not part of the template itself and there are no line numbers to preserve.
So it seems to me that we don't have any problem.

Or did I not understand you? Can you give an example?

One more point: this commit should probably be "Issue 5333" not a "Noissue".

Cheers,
Vasily

https://codereview.adblockplus.org/29947567/diff/29948568/tests/expected_outp...
File tests/expected_output/relative/de/rel_path (right):

https://codereview.adblockplus.org/29947567/diff/29948568/tests/expected_outp...
tests/expected_output/relative/de/rel_path:1: {{ head|safe }}
So what's happening here is that split_head_body() extracts the content of
<head> tag into `head`, removes the tag itself and the rest comes here as
`body`. If we want to produce a reasonable HTML on the output, one approach that
can work is to move <!DOCTYPE> and <html> tags into the template and then
include the head and the body into it:

    <!DOCTYPE html>
    <html>
        <head>
        {{ head | safe }}
        ... more head stuff from template, maybe some links ...
       </head>
       {{ body | safe }}
    </html>

 The page would then have those two tags at the top level without anything
around them:

    <head>
        ... head stuff ...
    </head>
    <body>
        ... body stuff ...
    </body>

Something like this should do.

https://codereview.adblockplus.org/29947567/diff/29948568/tests/test_site/pag...
File tests/test_site/pages/rel_path.html (right):

https://codereview.adblockplus.org/29947567/diff/29948568/tests/test_site/pag...
tests/test_site/pages/rel_path.html:6: {{ head|safe }}
This line probably shouldn't be here. It confused the hell out of me when I was
looking at the outputs ;)

rhowell

Hi Vasily and Winsley! Hope you had a nice time at the team week; the ...

Dec. 19, 2018, 2:33 a.m. (2018-12-19 02:33:31 UTC) #6

Vasily Kuznetsov

Hi Rosie, It seems that the team week was too good and this fell out ...

Jan. 4, 2019, 8:16 p.m. (2019-01-04 20:16:57 UTC) #7

rhowell

Jan. 7, 2019, 10:25 p.m. (2019-01-07 22:25:06 UTC) #8

Hi Vasily,

Thanks for finding this! I would have pinged you soon, if you didn't find it
first.  :)

https://codereview.adblockplus.org/29947567/diff/29964558/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29947567/diff/29964558/cms/converters.py#n...
cms/converters.py:329: text =
re.sub(r'(<[\w]+\s[^<>]*\b(href)=\")([^<>\"]+)(\")',
On 2019/01/04 20:16:56, Vasily Kuznetsov wrote:
> Perhaps at this point it makes sense to combine these two regexps into one,
with
> "(href|src)" in the middle, since they are identical except for just this one
> piece. What do you think?

Good idea, done.

Expand All Messages | Collapse All Messages