-
Notifications
You must be signed in to change notification settings - Fork 124
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
baseUrl's value sometimes wrong when referencing inner website's content #263
Conversation
Additional writeup: This is the content that MarkBind generates, before it processes the includes through # 262: Broken baseUrl links for inner websites
Version: v1.8.0.
## Include `inner_site/cheese.md`:
<div cwf="C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\markbind-problems\issue_262\index.md" include-path="C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\markbind-problems\issue_262\inner_site\cheese.md">
This is cheese's baseUrl: {{baseUrl}}.
</div>
## Include `inner_site/index.md`:
<div cwf="C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\markbind-problems\issue_262\index.md" include-path="C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\markbind-problems\issue_262\inner_site\index.md">
<div cwf="C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\markbind-problems\issue_262\inner_site\index.md" include-path="C:\Users\Tan Wang Leng\Documents\GitHub\yamgent\markbind-problems\issue_262\inner_site\cheese.md">
This is cheese's baseUrl: {{baseUrl}}.
</div>
</div>
## Expected correct rendering
This is cheese's baseUrl: /main_website/inner_site. The first This is because:
Hence, my PR fixes the two problems mentioned above. |
727a03a
to
1befaf7
Compare
@acjh if you have the time, do feel free to take a look at the PR. :) |
@nicholaschuayunzhi @acjh Just a heads up, will be getting this merged tomorrow morning, feel free to review this whenever you can. :) |
The current working file path (ATTRIB_CWF) specifies the document that is including another file (this included file's path is specified with ATTRIB_INCLUDE_PATH). When resolving references inside the included file, it should be resolved with regards to the included file's directory, NOT the working file's directory. For example, if the current document is 'abc/index.md', and it includes another document 'book/some-file.md', then resolving references in 'some-file.md' should be done with the 'book/' directory. not the 'abc/' directory. Let's use ATTRIB_INCLUDE_PATH instead of ATTRIB_CWF so that the baseUrl is not resolved wrongly.
Parser#_rebaseReference() may be called multiple times. The file content may already have {{hostBaseUrl}} in it due to previous calls to Parser#_rebaseReference(). The variable {{hostBaseUrl}} is still not defined at this stage. Undefined variables are replaced by nunjucks with nothing, resulting in wrong paths. Let's prevent nunjucks from replacing {{hostBaseUrl}}, by declaring the variable to be itself, so that the value can be injected later when it actually exist.
dd8e7e1
to
e3c5d6d
Compare
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Bug fix
Fixes #262.
What is the rationale for this request?
When including files belonging to an inner website, the computation of
baseUrl
is wrong if the depth of the inclusion is only 1.What changes did you make? (Give an overview)
Fixed the logic of the algorithm such that the computation of
baseUrl
is correct regardless of how deep the inclusion is.Provide some example code that this change will affect:
https://github.com/yamgent/markbind-problems/tree/master/issue_262
Is there anything you'd like reviewers to focus on?
The soundness of the fix.
Testing instructions:
issue_262
directory.markbind serve
.This is cheese's baseUrl: /main_website/inner_site.
a. In the buggy version, the first entry will show
This is cheese's baseUrl: /main_website.