Optional html blocks in markdown

[lovasoa]

No description provided.

[lovasoa]

I think this is overkill

[guspower]

I was trying to avoid testing with a full AppConfig. Do you have a better alternative in mind?

[lovasoa]

Thanks for the pr, @guspower ! I made some comments.

[lovasoa]

It's strange to use the path instead of the value. This means you cannot pass a variable to your helper. We should probably read the string value of the argument here.

[guspower]

This was based on my logging of what was going on with different combinations passed into the handler; I'll switch it to use the variable.

[lovasoa]

I think the syntax should look like

Suggested change

	{{{~markdown contents_md allow_unsafe~}}}
	{{{~markdown contents_md 'allow_unsafe'~}}}

(pass a string to the helper)

[lovasoa]

We don't want to take any existing safe code and make it unsafe. If we enable allow_unsafe somewhere, it should be in a new argument, with a big warning in the documentation.

[lovasoa]

We should also document the new options to the markdown helper: https://sql-page.com/custom_components.sql

[guspower]

We don't want to take any existing safe code and make it unsafe. If we enable allow_unsafe somewhere, it should be in a new argument, with a big warning in the documentation.

OK can you elaborate a bit further on this point so that I can better understand, specifically around the implementation details? If we use a hard coded string 'allow_unsafe' rather than reading a variable of the same name, this means that the user will have to create their own templates that supply that value rather than use the existing components? Sorry, I'm probably misunderstanding this point.

[lovasoa]

I mean that all sqlpage code that exists today should continue to work as is in the next version. So we cannot take an existing contents_md that is safe today and make it unsafe. Instead, we should add a new unsafe_contents_md property. Users can explicitly switch to it if they assess it's safe in their case.

[guspower]

I mean that all sqlpage code that exists today should continue to work as is in the next version. So we cannot take an existing contents_md that is safe today and make it unsafe. Instead, we should add a new unsafe_contents_md property. Users can explicitly switch to it if they assess it's safe in their case.

Right - gotcha; thanks for the explanation +1

[guspower]

OK I've updated the implementation and extended the docs. Let me know what you think.

[lovasoa]

@guspower , what about this:

diff --git a/src/template_helpers.rs b/src/template_helpers.rs
index 7be62c9..f6590c6 100644
--- a/src/template_helpers.rs
+++ b/src/template_helpers.rs
@@ -274,8 +274,6 @@ struct MarkdownHelper {
 }
 
 impl MarkdownHelper {
-    const ALLOW_UNSAFE: &'static str = "allow_unsafe";
-
     fn new(config: &impl MarkdownConfig) -> Self {
         Self {
             allow_dangerous_html: config.allow_dangerous_html(),
@@ -283,38 +281,39 @@ impl MarkdownHelper {
         }
     }
 
-    fn calculate_options(&self, args: &[PathAndJson]) -> markdown::Options {
-        let mut options = self.system_options();
-
-        if !options.compile.allow_dangerous_html && args.len() > 1 {
-            if let Some(arg) = args.get(1) {
-                if arg.value().as_str() == Some(Self::ALLOW_UNSAFE) {
-                    options.compile.allow_dangerous_html = true;
-                }
-            }
-        }
-
-        options
-    }
-
-    fn system_options(&self) -> markdown::Options {
+    fn system_options(&self, preset_name: &str) -> Result<markdown::Options, String> {
         let mut options = markdown::Options::gfm();
         options.compile.allow_dangerous_html = self.allow_dangerous_html;
         options.compile.allow_dangerous_protocol = self.allow_dangerous_protocol;
         options.compile.allow_any_img_src = true;
 
-        options
+        match preset_name {
+            "default" => {}
+            "allow_unsafe" => {
+                options.compile.allow_dangerous_html = true;
+                options.compile.allow_dangerous_protocol = true;
+            }
+            _ => return Err(format!("unknown markdown preset: {preset_name}")),
+        }
+
+        Ok(options)
     }
 }
 
 impl CanHelp for MarkdownHelper {
     fn call(&self, args: &[PathAndJson]) -> Result<JsonValue, String> {
-        let options = self.calculate_options(args);
-        let as_str = match args {
-            [v] | [v, _] => v.value(),
-            _ => return Err("expected one or two arguments".to_string()),
+        let (markdown_src_value, preset_name) = match args {
+            [v] => (v.value(), "default"),
+            [v, preset] => (
+                v.value(),
+                preset
+                    .value()
+                    .as_str()
+                    .ok_or("markdown template helper expects a string as preset name")?,
+            ),
+            _ => return Err("markdown template helper expects one or two arguments".to_string()),
         };
-        let as_str = match as_str {
+        let markdown_src = match markdown_src_value {
             JsonValue::String(s) => Cow::Borrowed(s),
             JsonValue::Array(arr) => Cow::Owned(
                 arr.iter()
@@ -326,7 +325,8 @@ impl CanHelp for MarkdownHelper {
             other => Cow::Owned(other.to_string()),
         };
 
-        markdown::to_html_with_options(&as_str, &options)
+        let options = self.system_options(preset_name)?;
+        markdown::to_html_with_options(&markdown_src, &options)
             .map(JsonValue::String)
             .map_err(|e| e.to_string())
     }

[lovasoa]

This produces more meaningful errors when the helper is not used as expected

[guspower]

Yep that looks good to me. My only comment would be to rename system_options to something like get_preset_options perhaps.

[lovasoa]

Thanks @guspower ! It's merged !