Conversation
2b72783 to
a830fe2
Compare
When baseurl in _config.yml is not empty, without this fix, it can be prepended more than once, which is incorrect. This fix adds logic to prevent prepending the baseurl when it is already a prefix in the url, but correctly handles cases where it is a prefix, but the url does not follow this prefix with a path/url separator. For example, when baseurl is set to /blog, but the url starts with /blogger it is not considered "prefixed" and /blog/ is prepended (resulting in /blog/blogger…, but if the url starts with /blog/it is considered "prefixed" and is not prepended again (keeping/resulting in/blog/…`) A limitation is that if the user supplied a (partial) URL starting with /blog/ and truly intended to produce /blog/blog/, it won't work. Fixes: cotes2020#2639 Signed-off-by: Dolf Starreveld <registration@starreveld.com>
a830fe2 to
d9de8d4
Compare
…riately Both home and post layouts did some of the work already done by _include/media-url.html. Since that _include was fixed to deal with the double prepend of baseurl, these layouts still had the problem. Change that so they use the include. Only effective when the previous PR is also merged. depends-on: PR cotes2020#2648 Signed-off-by: Dolf Starreveld <registration@starreveld.com>
cotes2020
left a comment
There was a problem hiding this comment.
Hi there, thanks for the PR and for your time!
From the first review, I’ve identified two issues:
remove_lastis a feature of Liquid 5.2.0, but Jekyll currently only supports Liquid 4.x.- There is a bug in this PR: when
site.cdnis not empty, the output media URL results in/<baseurl><cdn>/<subpath>.
|
Changes made. |
Where is it? Did you forget to push? |
1. remove_last is a feature of Liquid 5.2.0, but Jekyll currently only supports Liquid 4.x. Changed code to be 4.x compatible. 2. There is a bug in this PR: when site.cdn is not empty, the output media URL results in /<baseurl><cdn>/<subpath>. Fixed. Signed-off-by: Dolf Starreveld <registration@starreveld.com>
|
Oops, it should be there no. I am not very experienced with PRs. |
|
After looking deeper into the related issue #2639, I don't think this change/PR is necessary.
|
|
I've come up with a workaround: when using the Change {%- assign remain = include.remain | default: false -%}
{%- if url -%}
{%- unless url contains '://' or remain -%}
{%- comment -%}
Handle src URL using existing logic.
...
{%- endcomment -%}
{%- endunless -%}
{{- url -}}Usage:  <!-- output: <img src='[site.cdn]/[site.baseurl]/path/to/img'> -->
{: .remain } <!-- output: <img src='/[site.baseurl]/path/to/img'> --> |
Type of change
Description
When
baseurlin_config.ymlis not empty, without this fix, it can be prepended more than once, which is incorrect.This fix adds logic to prevent prepending the
baseurlwhen it is already a prefix in the url, but correctly handles cases where it is a prefix, but the url does not follow this prefix with a path/url separator.For example, when
baseurlis set to/blog, but the url starts with/bloggerit is not considered "prefixed" and/blog/ is prepended (resulting in/blog/blogger…, but if the url starts with/blog/it is considered "prefixed" and is not prepended again (keeping/resulting in/blog/…`)A limitation is that if the user supplied a (partial) URL starting with
/blog/and truly intended to produce/blog/blog/, it won't work.Additional context
Fixes #2639, and: