Make URL part joining aware of absolute URLs #2196
Conversation
mosra
force-pushed
the
absolute-url-merging
branch
2 times, most recently
from
4cf4a5b to
23c4135
Compare
mosra
force-pushed
the
absolute-url-merging
branch
2 times, most recently
from
7eaae71 to
4d4b31d
Compare
|
Update: looked again into this, rebased onto current master and fixed broken behavior with Thanks in advance! |
So it can be used from outside.
Previously, with RELATIVE_URLS disabled, when both SITEURL and
STATIC_URL were absolute, the final generate data URLs looked wrong like
this (two absolute URLs joined by `/`):
http://your.site/http://static.your.site/image.png
With this patch, the data URLs are correctly:
http://static.your.site/image.png
This also applies to all *_URL configuration options (for example,
ability to have pages and articles on different domains) and behaves
like one expects even with URLs starting with just `//`, thanks to
making use of urllib.parse.urljoin().
However, when RELATIVE_URLS are enabled, urllib.parse.urljoin() doesn't
handle the relative base correctly. In that case, simple os.path.join()
is used. That, however, breaks the above case, but as RELATIVE_URLS are
meant for local development (thus no data scattered across multiple
domains), I don't see any problem.
Just to clarify, this is a fully backwards-compatible change, it only
enables new use cases that were impossible before.
mosra
force-pushed
the
absolute-url-merging
branch
from
4d4b31d to
0b13aa9
Compare
|
Oh, I did the same (rebased and triggered the build) after seeing the commit, because I saw it earlier than your comment :) Thank you a lot! |
|
I have a bunch of new features (such as cache busting for static files etc) that depend on this PR (and mainly 1f30306 in particular -- it just moves a function around, nothing more), would it be possible to get this merged anytime soon so I can rely on something that's already considered stable? Thank you a lot. |
|
Apologies for the delay, @mosra. Any comments from @getpelican/reviewers? |
|
Many thanks for the contribution and for your patience, Vladimír. Much appreciated! |
|
Thanks for the merge! :) |
Previously, with
RELATIVE_URLSdisabled, when bothSITEURLandSTATIC_URLwere absolute, the final generated data URLs looked wrong like this (two absolute URLs joined by/):With this patch, the data URLs are correctly:
This also applies to all
*_URLconfiguration options (for example, ability to have pages and articles on different domains) and behaves like one expects even with URLs starting with just//, thanks to making use ofurllib.parse.urljoin().However, when
RELATIVE_URLSare enabled, urllib.parse.urljoin() doesn't handle the relative base correctly. In that case, simpleos.path.join()is used. That, however, breaks the above case, but asRELATIVE_URLSare meant for local development (thus no data scattered across multiple domains), I don't see any problem.Just to clarify, this is a fully backwards-compatible change, it only enables new use cases that were impossible before.
Properly implements, obsoletes and thus closes #2031 and #2034. Also, besides doing the above, there's a commit that moves the local link replacer function to a
_link_replacer()function that's callable from outside -- a minimal subset of #2164 that should not do any harm.Would be nice if this patch could still make it into 3.8, I'm depending on it quite heavily. Thank you a lot!