Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trouble escaping a Hugo paired shortcode #5902

Closed
maelle opened this issue Jun 13, 2023 · 21 comments · Fixed by #5907
Closed

Trouble escaping a Hugo paired shortcode #5902

maelle opened this issue Jun 13, 2023 · 21 comments · Fixed by #5907
Assignees
Labels
bug Something isn't working triaged-to Issues that were not self-assigned, signals that an issue was assigned to someone.
Milestone

Comments

@maelle
Copy link
Contributor

maelle commented Jun 13, 2023

Bug description

We have a Quarto book where we document how to write blog posts on our Hugo website. With the pre-release version of Quarto (and the released version, but not with 1.3.49 that I have locally) the build fails because of the chunk below:

```{shortcodes=false}

{{< gallery >}}
{{< figureforgallery src="search2.png" alt="Screenshot of r-universe simple search results." >}}
{{< figureforgallery src="search3.png" alt="Screenshot of r-universe advanced search query." >}}
{{< figureforgallery src="search4.png" alt="Screenshot of r-universe advanced search results." >}}
{{< /gallery >}}
```

The error is ERROR: invalid shortcode: /gallery.

Steps to reproduce

I've set up a reprex at https://github.com/maelle/quartohugo It's the default Quarto book created with RStudio IDE (Create New Project > Quarto book) except I tweaked https://github.com/maelle/quartohugo/blob/main/index.qmd to include the faulty chunk.

I added a GHA workflow https://github.com/maelle/quartohugo/blob/main/.github/workflows/render.yaml The latest log is at https://github.com/maelle/quartohugo/actions/runs/5253624754/jobs/9491184540

Expected behavior

The book should be built and the shortcode should appear as is in code.

Actual behavior

There is an error ERROR: invalid shortcode: /gallery.

Your environment

ubuntu-latest on GHA

Quarto check output

I'm using the official Quarto GitHub Action so hopefully it's fine I don't answer this!

@cderv
Copy link
Collaborator

cderv commented Jun 13, 2023

@mcanouil it is not really Hugo related. I don't know what the hugo tag means, but it is not related to the Hugo output format. I don't think it applies here.

This is about shortcodes itself. The stack trace is

ERROR: invalid shortcode: /gallery

Stack trace:
    at parseShortcode (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/core/lib/parse-shortcode.ts:91:11)
    at isBlockShortcode (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/core/lib/parse-shortcode.ts:19:12)
    at breakQuartoMd (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/core/lib/break-quarto-md.ts:189:28)
    at async validateDocumentFromSource (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/core/schema/validate-document.ts:34:14)
    at async renderFiles (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/command/render/render-files.ts:320:34)
    at async render (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/command/render/render-shared.ts:98:18)
    at async renderForPreview (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/command/preview/preview.ts:400:24)
    at async render (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/command/preview/preview.ts:159:22)
    at async preview (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/command/preview/preview.ts:176:18)
    at async Command.fn (file:///C:/Users/chris/DOCUME~1/DEV_R/QUARTO~2/src/command/preview/cmd.ts:356:7)

The issue is that we are parsing shortcode during our Qmd processing in Typescript before any Lua is happening. And so the escaping of shortcode does not apply using the attributes.

parseShortcode is applied anyway, and {{< /gallery >}} is not a valid shortcode.

If we use the shorter syntax using triple curly braces, there is no issue.

---
title: "Sidenotes"
format: html
---

```md
{{{< gallery >}}}
{{{< figureforgallery src="search2.png" alt="Screenshot of r-universe simple search results." >}}}
{{{< figureforgallery src="search3.png" alt="Screenshot of r-universe advanced search query." >}}}
{{{< figureforgallery src="search4.png" alt="Screenshot of r-universe advanced search results." >}}}
{{{< /gallery >}}}
```

We document escaping of shortcodes here: https://quarto.org/docs/extensions/shortcodes.html#escaping

It seems to be the only method that works.

@cscheid do you think the ```{shortcodes=false} attributes can be taken into account to avoid parse and check the inside code block shortcode syntax that is meant to be escaped ? I am thinking it could be possible to do the parseShortcode on all line except on those inside a code blocks, but not sure we match / check attributes in the parser.

@cderv cderv added the triaged-to Issues that were not self-assigned, signals that an issue was assigned to someone. label Jun 13, 2023
@mcanouil
Copy link
Collaborator

@cderv I figured that much while testing on the repo.
From the code, any shortcodes inside ```{shortcodes=false} should not even be parsed in the first place, but for some reason Quarto is still trying to parse them (even if it does not evaluate them).

@mcanouil mcanouil removed the hugo label Jun 13, 2023
@cderv
Copy link
Collaborator

cderv commented Jun 13, 2023

From the code, any shortcodes inside ```{shortcodes=false} should not even be parsed in the first place, but for some reason Quarto is still trying to parse them (even if it does not evaluate them).

It is not that simple I think. ```{shortcodes=false} is for a Lua processing, and this works. But Quarto works by first parsing the .qmd file with an internal parser that will "break" the content into blocks, and lines and do some checks and processing before rebuilding document to be passed to knitr or jupyter for computation, or pandoc for conversion. (probably simple explanation).

The problem is that our parsing breakQuartoMd() does check and skip {{{ ... }}} but ignore the attributes on code blocks (because it does really acts as Lua with an AST).

@cscheid is the one knowing the internal of all this and IMO can say if known limitation or an issue to fix

@mcanouil
Copy link
Collaborator

mcanouil commented Jun 13, 2023

Note that the regex, in the following code, does not allow a shortcode to start with \ (or. /, etc.), which lead to the error (but might not be the root cause I believe).

const nameMatch = capture.match(/^[a-zA-Z0-9_]+/);

This changes was implemented in #4009
Last working version is 1.3.115
First non working version is 1.3.117

Weirdly, there is no 1.3.116 tag

@cderv
Copy link
Collaborator

cderv commented Jun 13, 2023

Interesting history !

Though I don't think this is the issue here, or at least the question to be answered. Let me clarify as I don't want this issue to be misunderstood.

Note that the regex, in the following code, does not allow a shortcode to start with \ (or. /, etc.), which lead to the error (but might not the root cause I believe).

I don't think shortcode with leading / are considered valid. We could make it valid as this could be an oversight, and changing the regex will indeed also solve this issue (or hide it, depending how we consider the below).

Even if this was valid, should the shortcode validity be checked when inside a shortcode-escaped codeblock ?

So under the assumption, the regex is ok., this is expected to be invalid here. The .qmd document aims to illustrate how to use hugo shortcodes which uses the same syntax as Quarto one (or more the other way around).

So the error is ok. There should be an error if {{< /gallery >}} was trying to be used as a Quarto shortcode. But it is not here - it is among an escaped CodeBlock and not intended to be process as shortcode.

However, our parser will check each line, and parseShortcode() is run leading to the error shown.

Should this really be run also on codeblocks with our shortcodes=false attribute ? That is the question I believe.

@mcanouil
Copy link
Collaborator

Should this really be run also on codeblocks with our shortcodes=false attribute ? That is the question I believe.

Exactly, this is what I meant in #5902 (comment)

Though I don't think this is the issue here, or at least the question to be answered. Let me clarify as I don't want this issue to be misunderstood.

Thus 😉

(but might not be the root cause I believe).

The change in the regex is the reason it was "working" before and it is no longer working.

@maelle
Copy link
Contributor Author

maelle commented Jun 13, 2023

By the way, I can imagine a code block with escaped shortcodes, where one would willingly present wrong shortcodes for teaching what not to do. So that'd be an argument for not checking shortcodes when shortcodes=false. 😉

@cscheid cscheid added this to the v1.4 milestone Jun 13, 2023
@cderv

This comment was marked as outdated.

@cscheid
Copy link
Collaborator

cscheid commented Jun 13, 2023

Ok, I'm having a slow day, I apologize. The bug is where @cderv said originally... I'm on it, will fix soon.

@cscheid
Copy link
Collaborator

cscheid commented Jun 13, 2023

PR open. Thanks for the report, the fix should be in soon!

cscheid added a commit that referenced this issue Jun 14, 2023
* support closing shortcodes cf hugo syntax. Closes #5902
* smoke test
* changelog
* changelog
@maelle
Copy link
Contributor Author

maelle commented Jun 15, 2023

Thank you!!

@lwasser
Copy link

lwasser commented Sep 17, 2023

hey there all! i'm running into this same issue as @maelle using quarto (with hugo) for pyOpenSci.
it was working fine until i just updated to 1.3.450 (i was on 1.2.x previously) to address some code output issues i was getting with figures (they were outputting with html src into markdown files and i found an issue about this and decided to upgrade).

I thought that "protecting" the shortcodes with ```{=markdown} might work as it did in 1.2 but that doesn't seem to be the case. applying triple brackets {{{ shortcode here }}} does work but will i have to go in and fix every single shortcode now and add another set of brackets??

{{< noticeowl "learn" >}}

basic text is here in markdown format
it it several lines so i need to close the shortcode after

{{< /noticeowl >}}

Is there perhaps another trick to ensuring this works? i didn't see anything in the quarto docs.

@cscheid
Copy link
Collaborator

cscheid commented Sep 17, 2023

@lwasser I apologize. This used to be a quarto "bug", where we ignored shortcodes inside code blocks. Unfortunately, some users need those shortcodes to be resolved, and so we fixed that bug. But you clearly have a document with lots of shortcodes that you don't want to process in code contexts. I don't think we have a way to ignore those, but I see the value in implementing one. Can you share a full document where this comes up so that we can figure out how to fix this properly? Thanks.

@lwasser
Copy link

lwasser commented Sep 22, 2023

hey there @cscheid thank you. i am actually having a handful of issues with my quarto builds. i did email Christophe about it just so you know. should i open a new issue? the PR is here - data-science-skills/dss-web#8 i'm essentially rebuilding a pure quarto site into a quarto/hugo site for flexibility. but i'm running into a lot of issues with images not rendering, shortcode issues, etc. some of these things worked in older versions of quarto and are broken now.

the pr is here - data-science-skills/dss-web#8 please let me know if i should open a new issue somewhere. it seems like the shortcode issue is fixed by BOTH using the {=markdown} safetly net (maybe i don't need that anymore?) and triple {{{ but only for the closing shortcode tag. that closing element seems to be what causes issues - perhaps it's being read as an escaped item?

@cscheid
Copy link
Collaborator

cscheid commented Sep 22, 2023

I confess it's going to be hard for me to go through the 594 changed files in that PR, sorry! The smaller the broken example, the faster we can get it fixed.

@cderv
Copy link
Collaborator

cderv commented Sep 22, 2023

@lwasser I'll get back to you next week about the PR and all your issues !

@lwasser
Copy link

lwasser commented Sep 22, 2023

Ok thank you all! I understand that is a huge pr. It's essentially moving the site from quarto to quarto hugo. So it includes theme files and such. But I can't merge it until the build actually works so the website builds.

Thank you @cderv for the reply! I'll hang tight and if I can provide something specific for you please say the word!!

@cderv
Copy link
Collaborator

cderv commented Oct 2, 2023

Unfortunately, some users need those shortcodes to be resolved, and so we fixed that bug. But you clearly have a document with lots of shortcodes that you don't want to process in code contexts. I don't think we have a way to ignore those, but I see the value in implementing one.

@cscheid this is the type of content in @lwasser website

{{< noticeowl "learn" >}}

After completing this page, you will be able to:

* Understand how dependency management can play a large role in **Python** programming.
* Explain how to use **conda** environments to manage your third party libraries.

{{< /noticeowl >}}

Those are shortcode for Hugo.

Escaping using {{{< .. >}}} works, but there is no block syntax for it.

Maybe we could bring an equivalent to ```{shortcodes=false} but without the code block part ?
Maybe

::: {shortcodes=false}
content 
::: 

this would deactivate shortcode parsing by Quarto in the whole block.

Would it be possible ?

No matter how we do it, this will be common in .qmd when format: hugo to have a way to escape shortcodes.

Also ur docs says (https://quarto.org/docs/output-formats/hugo.html#shortcodes)

Note that Hugo shortcodes and Quarto shortcodes share the same basic syntax (e.g. {{< var foo >}}). This is normally not a problem as shortcodes not recognized by Quarto are passed through unmodified to Hugo.

But it does not seem to be the case anymore. I'll open an issue about that one... (see #7108)

@lwasser
Copy link

lwasser commented Oct 2, 2023

would it be possible to update this page - in the docs to suggest using

::: {shortcodes=false}
content 
::: 

instead of

```{=markdown}
[click here]()
```

or maybe to clarify as perhaps i just read that page incorrectly? many thank y'all.

@cderv
Copy link
Collaborator

cderv commented Oct 2, 2023

To be clear, this does not exists yet

::: {shortcodes=false}
content 
:::

and this

```{=markdown}
[click here]()
```

is still useful to keep some Markdown content, unprocessed by Pandoc.

This is a bit different than shortcode... We'll update the doc anyway depending on #7108

@cscheid
Copy link
Collaborator

cscheid commented Oct 2, 2023

I'll set aside some time this week to fix this. The problem is that shortcode parsing is quite brittle because it needs to happen ahead of Pandoc parsing (it's in qmd-reader.lua, effectively). It's not easy code to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged-to Issues that were not self-assigned, signals that an issue was assigned to someone.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants