Item15053: Incorrect href for external references and attachments with query
Priority: Normal
Current State: Closed
Released In: n/a
Target Release: n/a
--
BramVanOosterhout - 21 Nov 2021
Pushed to github. Package to be released 14 Dec 2021. Done 19 Dec.
PublishPlugin with copyexternal=0
translates
from this | to this |
<a href="https://foswiki.org" >
|
<a href="..">
|
<script class='script JQUERYPLUGIN::COMMENT' src='/devwiki/pub/System/CommentPlugin/comment.js?version=3.0'></script>
|
<script class="script JQUERYPLUGIN::COMMENT" src="../http:/cdl/devwiki/pub/System/CommentPlugin/comment.js?version=3.0"></script>
|
The first issue
Losing the external reference.
These problems are related to the semantics of the
URI
module. The URI module allows the path to be an empty string (length==0). That leads to the
no path in
logic and hence no URL. Line 1027-1029:
if ( !defined $url->path() || length( $url->path() ) == 0 ) {
$this->logDebug("- no path in ");
Adding a path if the scheme exists and the path does not exist solves the problem
$url->path('/') if $url->scheme() && !$url->path();
if ( !defined $url->path() || length( $url->path() ) == 0 ) {
$this->logDebug("- no path in ");
Or so I thought, but the result is:
<a href="../https:/foswiki.org"> fail </a>
The problem has moved to line 989,990:
_processURL( $attrs{$key}, "$web.$topic" );
unless ( $new eq $attrs{$key} || $new =~ /^#/ ) {
The problem is that
_processURL
returns:
- a string: Lines: 1036, 1041 and others
- a uri object: through
_processexternalURL
and copyexternal=0
So the comparison at line 990 will fail on the uri object. Extending the comparison to:
unless (( blessed( $new ) && $new->eq( $attrs{$key} ) )
|| $new eq $attrs{$key} || $new =~ /^#/ ) {
Where
blessed
is obtained with
use Scalar::Util qw( blessed );
This approach certainly solves the problem, but I wonder whether the issue is that line 1013:
$url = URI->new($url);
changes the content of the
$url
variable from a string to a uri object. And maybe that change was not carried through to _processExternalURL? This is doing my head in.
The second issue
Making absolute references relative.
A third attempt (7 Dec 2021)
I have updated Publisher.pm along the lines described below. I have also updated Publisher so that urls written as:
%SCRIPTURL{"viewfile" web="%WEB%" topic="MyTargetTopic" }%/MyTestAttachment.txt?rev=1
are handled correctly. Now changing the Apache config so that URLs of the form:
%PUBURL{ web="%WEB%" topic="MyTargetTopic" }%/MyTestAttachment.txt?rev=1
are rewritten by Apache to access through viewfile and hence are retrieved as expected.
A second attempt
(See discussion with
MichaelTempest on
foswiki-discuss)
Pub resources with a ?query parameter are treated as external resources to ensure that ONLY the referred version of the attachment is published.
- The retrieval is for
{foswiki}/attachment.ext?version=N
- The copy of the requested version (N) is saved as
{publish}/attachment.ext
- And the URL rewritten as
{publishurl}/attachment.ext
(no query. There is only one attachment. The selected version)
This works when
copyexternal=1
And fails with
copyexternal=0
. Probably because line 1300 stops handling of external URLs:
1297 sub _processExternalResource {
1298 my ( $this, $url ) = @_;
1299
1300 return $url unless $this->{opt}->{copyexternal};
...
Following this train of thought, I changed the code at lines 1186-1191 to:
if ( $type eq 'pub' ) {
$attachment = pop(@$upath) if scalar(@$upath);
$topic = pop(@$upath) if scalar(@$upath);
$web = join( '/', @$upath );
if ( !$url->query() ) {
$new = $this->_processInternalResource( $web, $topic, $attachment );
}
else {
#the attachment is versioned. Retrieve the correct version.
# use an absolute URL, which we can do safely.
$url = URI->new(
Foswiki::Func::getPubUrlPath( $web, $topic, $attachment,
absolute => 1 )
. '?'
. $url->query()
);
$new = $this->_processExternalResource($url);
}
}
Now the pub url is evaluated
- no query: process as internal resource
- query: process as external resource
I took the bar
return $url unless $this->{opt}->{copyexternal};
out of
_processExternalResource
, so
_processExternalResource
will work regardless of the
copyexternal
option.
And I replaced lines: 1224-1247 with:
else {
# It's a real external resource
$this->logDebug("- external resource");
return $url unless $this->{opt}->{copyexternal};
$new = $this->_processExternalResource($url);
}
The
git diff
included
below.
This logic seems to be more in line with expectations, and small tests indicate the correct result is achieved.
I will do some extensive testing, but it is a start.
Failed resolution
The text below is for historical reasons only.
The attempt below cures the symptoms, but not the cause. And it ignores the reason for treating attachments with ?query special:
versioned attachments!!
is caused by treating pub resources with a query as external resources. Lines 1185,1186
# Is it a pub resource? With no associated query?
if ( $type eq 'pub' && !$url->query() ) {
The comment suggests this is deliberate, but it does not get the desired result. I cannot see a rationale for it, so I have changed the code to:
if ( $type eq 'pub' ) {
$attachment = pop(@$upath) if scalar(@$upath);
$topic = pop(@$upath) if scalar(@$upath);
$web = join( '/', @$upath );
$new = $this->_processInternalResource( $web, $topic, $attachment );
$new .= '?'.$url->query() if $url->query();
}
The query is ignored in the processing of the url and appended if it existed in the original url. It seems logical, but the comment leaves me uneasy.
Hmm.
The changes for the first issue look fine to me.
I don't see how the code for the second issue relates to the second issue, but
there are use-cases for a pub URL with a query:
http://example.com/path/to/pub/web/topic/attachment.ext?t=1234 . It allows you to provide a URL in a published version that always links to the latest version.
I can think of a few uses for that.
- If you are using Foswiki as a CMS, and you are using PublishPlugin to take snapshots for release, and you want your HTML snapshot to always refer to the latest copy of something (e.g. a document template) for some reason
- If your application has some external script that updates an attachment automatically, you may wish to refer to the latest copy rather than take a snapshot.
With regard to your second example, note that it is referencing a specific version of an attachment. In this case, the query must be preserved, and the code to do this is in line 1280 in
https://github.com/foswiki/PublishPlugin/commit/86d00745d357ebe2eafce58acabf7a27fb34742f - at least it is for the relative URL case.
I see this change came in
Foswikitask:Item14614: support parameters passed to resource URLs
https://github.com/foswiki/PublishPlugin/commit/86d00745d357ebe2eafce58acabf7a27fb34742f
If you look at about line 1186, you have
if ( $type eq 'pub' ) {
...
$new = $this->_processInternalResource( $web, $topic, $attachment );
- there is another problem. _processInternalResource only makes sense if it is a pub link without a query, because the description of _processInternalResource is "Copy a resource from pub (image, style sheet, etc.) to the archive. Return the path to the copied resource in the archive." - so appending the query doesn't make sense. What should happen instead is that the query should be taken into account. This happens
here but it is only applied to relative links, and that may be a bug.
--
MichaelTempest - 21 Nov 2021
git diff lib/Foswiki/Plugins/PublishPlugin/Publisher.pm
Here is the git diff for those that are interested
diff --git a/lib/Foswiki/Plugins/PublishPlugin/Publisher.pm b/lib/Foswiki/Plugins/PublishPlugin/Publisher.pm
index bbadc5c..5b97504 100644
--- a/lib/Foswiki/Plugins/PublishPlugin/Publisher.pm
+++ b/lib/Foswiki/Plugins/PublishPlugin/Publisher.pm
@@ -8,6 +8,7 @@ use Foswiki::Func;
use Error ':try';
use Assert;
use URI ();
+use Scalar::Util qw( blessed );
require Exporter;
our @ISA = qw(Exporter);
@@ -987,7 +988,8 @@ sub _rewriteTag {
}
return $tag unless $attrs{$key};
my $new = $this->_processURL( $attrs{$key}, "$web.$topic" );
- unless ( $new eq $attrs{$key} || $new =~ /^#/ ) {
+ unless (( blessed( $new ) && $new->eq( $attrs{$key} ) )
+ || $new eq $attrs{$key} || $new =~ /^#/ ) {
#$this->logDebug("Rewrite $new (rel to ",
# $this->{archive}->getTopicPath( $web, $topic ).')');
@@ -1023,6 +1025,8 @@ sub _processURL {
# $url->frag
$this->logDebug( "Processing URL ", $url );
+
+ $url->path('/') if $url->scheme() && !$url->path();
if ( !defined $url->path() || length( $url->path() ) == 0 ) {
@@ -1182,12 +1186,26 @@ sub _processURL {
my $attachment;
my $new = $url;
- # Is it a pub resource? With no associated query?
- if ( $type eq 'pub' && !$url->query() ) {
+ # Is it a pub resource?
+ if ( $type eq 'pub' ) {
+
$attachment = pop(@$upath) if scalar(@$upath);
$topic = pop(@$upath) if scalar(@$upath);
$web = join( '/', @$upath );
- $new = $this->_processInternalResource( $web, $topic, $attachment );
+ if ( !$url->query() ) {
+ $new = $this->_processInternalResource( $web, $topic, $attachment );
+ }
+ else {
+ #the attachment is versioned. Retrieve the correct version.
+ # use an absolute URL, which we can do safely.
+ $url = URI->new(
+ Foswiki::Func::getPubUrlPath( $web, $topic, $attachment,
+ absolute => 1 )
+ . '?'
+ . $url->query()
+ );
+ $new = $this->_processExternalResource($url);
+ }
}
elsif ( $type eq 'script' ) {
@@ -1227,22 +1245,7 @@ sub _processURL {
# process it as an external resource
$this->logDebug("- external resource");
- if ( $type eq 'pub' && $is_rel ) {
-
- # Relative URLs with queries can't be retrieved using
- # Foswiki::Func::getExternalResource; but it works if
- # we convert to an absolute URL, which we can do safely.
- $attachment = pop(@$upath) if scalar(@$upath);
- $topic = pop(@$upath) if scalar(@$upath);
- $web = join( '/', @$upath );
- $url = URI->new(
- Foswiki::Func::getPubUrlPath( $web, $topic, $attachment,
- absolute => 1 )
- . '?'
- . $url->query()
- );
- }
-
+ return $url unless $this->{opt}->{copyexternal};
$new = $this->_processExternalResource($url);
}
@@ -1297,8 +1300,6 @@ sub _processInternalResource {
sub _processExternalResource {
my ( $this, $url ) = @_;
- return $url unless $this->{opt}->{copyexternal};
-
return $this->{copied_resources}->{$url}
if ( $this->{copied_resources}->{$url} );