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

Footnote ID prefix and footnote ref ID prefix config options sometimes ignored #524

Closed
bakerkretzmar opened this issue Jul 20, 2020 · 3 comments · Fixed by #530
Closed
Labels
bug Something isn't working right fixed Fix has been implemented

Comments

@bakerkretzmar
Copy link
Contributor

Version(s) affected: 1.5.x

Description

The footnote/ref_id_prefix and footnote/footnote_id_prefix configuration options are only used to generate element id attributes, but are ignored for the href attributes corresponding to those IDs, breaking all footnote reference links and backlinks if either config option is customized.

The default values for these configuration options, fnref: and fn:, are hard-coded in 5 places:

$backrefs = $document->getData('#fn:' . $node->getReference()->getDestination(), []);

'#fnref:' . $existingReference->getLabel(),

return new Reference($refLabel, '#fn:' . $refLabel, $label);

return new Reference($label, '#fn:' . $label, $label);

How to reproduce

Config:

    'footnote' => [
        'ref_id_prefix' => 'customfnref:',
        'footnote_id_prefix' => 'customfn:',
    ],

Markdown:

Here[^note1]

[^note1]: There

Expected HTML:

<p>Here<sup id="customfnref:note1"><a class="custom-ref" href="#customfn:note1" role="doc-noteref">1</a></sup></p>
<div class="custom-notes" role="doc-endnotes">
    <ol>
        <li class="custom-footnote" id="customfn:note1" role="doc-endnote">
            <p>There&nbsp;<a class="custom-backref" rev="footnote" href="#customfnref:note1" role="doc-backlink"></a></p>
        </li>
    </ol>
</div>

Actual HTML:

<!-- Correct id, wrong href -->
<p>Here<sup id="customfnref:note1"><a class="custom-ref" href="#fn:note1" role="doc-noteref">1</a></sup></p>
<div class="custom-notes" role="doc-endnotes">
    <ol>
        <!-- Correct id -->
        <li class="custom-footnote" id="customfn:note1" role="doc-endnote">
            <!-- Wrong href -->
            <p>There&nbsp;<a class="custom-backref" rev="footnote" href="#fnref:note1" role="doc-backlink"></a></p>
        </li>
    </ol>
</div>

Possible Solution

Instead of hard-coding these values, we can update those 5 references to them to check the config. I'm not sure how involved this would be since it looks to me like none of those classes currently have access to the config at all.

But also, since these attributes are completely hidden from end users and don't affect styling, it occurred to me that they don't necessarily need to be customizable? An easier solution would be to hard-code them everywhere and not allow them to be overriden at all.

Thoughts? Either way, I'll try to get a PR in to fix this in the next week or two!

@markhalliwell markhalliwell added the bug Something isn't working right label Jul 20, 2020
@markhalliwell
Copy link
Contributor

But also, since these attributes are completely hidden from end users and don't affect styling, it occurred to me that they don't necessarily need to be customizable? An easier solution would be to hard-code them everywhere and not allow them to be overriden at all.

Unfortunately, this isn't a viable solution. In fact, one of the entire reasons behind merging the footnotes extension into this project was to give it customizable configuration (rezozero/commonmark-ext-footnotes#7). This was, in part, due to discovery in https://www.drupal.org/project/markdown/issues/3131224 of a Drupal core bug (https://www.drupal.org/project/drupal/issues/2544110) which stripped the colon : from the element identifiers as part of its HTML sanitization.

Logically, it makes sense to fix said Drupal core bug (and I'm sure it will one day... a Drupal bug half-life varies wildly), but practically, there could be any number of reasons why these identifiers may need to be configurable (maybe integration with a custom SPA app/3rd-party JS library of some kind).

@bakerkretzmar
Copy link
Contributor Author

@MarkCarver thanks for that context, makes sense! I'll work on fixing the bug to make them fully customizable.

@close-label close-label bot added the fixed Fix has been implemented label Aug 15, 2020
@markhalliwell
Copy link
Contributor

Hm. Not sure why this didn't get closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right fixed Fix has been implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants