From 2c964092d6c3022cf13b51ff65dbdf3c305238f2 Mon Sep 17 00:00:00 2001 From: Giuseppe Bilotta Date: Tue, 22 Feb 2011 23:20:35 +0100 Subject: [PATCH] Reply to reviews --- .../feed_enhancements_for_inline_pages.mdwn | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/doc/todo/feed_enhancements_for_inline_pages.mdwn b/doc/todo/feed_enhancements_for_inline_pages.mdwn index ec7c8c668..bfc7531bb 100644 --- a/doc/todo/feed_enhancements_for_inline_pages.mdwn +++ b/doc/todo/feed_enhancements_for_inline_pages.mdwn @@ -28,6 +28,8 @@ already been included upstream. > Applied. --[[Joey]] +>> Thanks. + The second patch, ‘inline: improve feed title and description management’, aligns feed title and description management by introducing a `title` option to complement `description`, and by basing the @@ -43,6 +45,17 @@ title and wiki name rather than hard-coding the wiki name as description. > > Oh, and could you indent your `elsif` the same as I? --[[Joey]] +>> I hadn't even realized that I was nesting ifs inside else clauses, +>> sorry. I think you're also right about the safety of the key, after +>> all it only gets interpolated with known, safe strings. + +>> The question is what to do for pages that do not have a description +>> (and are not the index). With your proposal, the Atom feed subtitle +>> would turn up empty. We could make it conditional in the default +>> template, or we could have `$desc` default to `$title` if nothing +>> else is provided, but at this point I see no reason to _not_ allow +>> the user to choose a way to build a default description. + The third patch, ‘inline: allow assigning an id to postform/feedlink’, does just that. I don't currently use it, but it can be particularly useful in the postform case for example for scriptable management of @@ -50,6 +63,8 @@ multiple postforms in the same page. > Applied. --[[Joey]] +>> Thanks. + In one of my wiki setups I had a terminating '/' in `$config{url}`. You mention that it should not be present, but I have not seen this requirement described anywhere. Rather than restricting the user input, @@ -60,6 +75,9 @@ created by `urlto()` by fixing the routine itself. > every call to `urlto`. And I'm not sure this is a comprehensive > fix to every problem a trailing slash in the url could cause. --[[Joey]] +>> Maybe something that sanitizes the config value would be better instead? +>> What is the policy about automatic changing user config? + The inline plugin is also updated (in a separate patch) to use `urlto()` rather than hand-coding the feed urls. You might want to keep this change even if you discard the urlto patch. @@ -67,3 +85,16 @@ change even if you discard the urlto patch. > IIRC, I was missing a proof that this always resulted in identical urls, > which is necessary to prevent flooding. I need such a proof before I can > apply that. --[[Joey]] + +>> Well, the URL would obviously change if the `$config{url}` ended in +>> slash and the `urlto` patch (or other equivalent) went into effect. + +>> Aside from that, if I read the code correctly, the only other extra +>> thing that `urlto` does is to `beautify_url_path` the `"/".$to` part, +>> and the only way this would cause the url to be altered is if the +>> feed name was "index" (which can easily happen) and +>> `$config{htmlext}` was set to something like `.rss` or +>> `.rss.1`. + +>> So there is a remote possibility that a different URL would be +>> produced. -- 2.39.2