You are here: Foswiki>Tasks Web>AllOutStandingItems>PublishPlugin>Item15053 (19 Dec 2021, BramVanOosterhout)Edit Attach

Item15053: Incorrect href for external references and attachments with query

pencil
Priority: Normal
Current State: Closed
Released In: n/a
Target Release: n/a
Applies To: Extension
Component: PublishPlugin
Branches:
Reported By: BramVanOosterhout
Waiting For:
Last Change By: BramVanOosterhout
-- 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} );

ItemTemplate edit

Summary Incorrect href for external references and attachments with query
ReportedBy BramVanOosterhout
Codebase 2.1.6
SVN Range
AppliesTo Extension
Component PublishPlugin
Priority Normal
CurrentState Closed
WaitingFor
Checkins
TargetRelease n/a
ReleasedIn n/a
CheckinsOnBranches
trunkCheckins
masterCheckins
ItemBranchCheckins
Release02x01Checkins
Release02x00Checkins
Release01x01Checkins
Edit  | Attach | Print version | History: r8 < r7 < r6 < r5 | Backlinks | View wiki text | Edit wiki text | More topic actions
Topic revision: r8 - 19 Dec 2021, BramVanOosterhout
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy