The documentation provided in the pathologic.api.php file includes an example for linking to an external CDN. However, the external option does not appear to be referenced or implemented in the pathologic.module file. When using this example code, the resulting URL looks something like http://domain.com/http%3A//cdn.example.com/image.png

Here's the culprit in pathologic.api.php:

  // If it's a path to a local image, make sure it's using our CDN server.
  if (preg_match('~\.(png|gif|jpe?g)$~', $url_params['path'])) {
    $url_params['path'] = 'http://cdn.example.com/' . $url_params['path'];
    $url_params['options']['external'] = TRUE;
  }
CommentFileSizeAuthor
#6 3027696-6.patch2.45 KBdww
#6 3027696-6.test-only.patch1.34 KBdww
#5 3027696-5.patch1.12 KBdww
#2 hook_pathologic_alter_external_option_doesnt_work_3027696-1.patch1.02 KBspleenboy

Comments

spleenboy created an issue. See original summary.

spleenboy’s picture

spleenboy commented
StatusFileSize
new hook_pathologic_alter_external_option_doesnt_work_3027696-1.patch1.02 KB

Here's a patch that corrects the issue by testing for the $url_params['options']['external']boolean and conditionally excludes the base:// prefix from the uri. Apologies if this patch isn't the right format. It's my first official contribution to any Drupal modules!

dww’s picture

dww
we/he/they
commented
Component: Documentation » Code
Status: Active » Needs review

Thanks @spleenboy! Looks like this isn't just a bug in the documentation, but that the code itself isn't invoking the hook that the docs describe. Since you uploaded a solution, you could mark this "needs review" in the hopes that someone will notice and move this forward.

Anyway, apologies I didn't look earlier. I forgot I agreed to co-maintain this module, and haven't checked the issue queue in ages. Doing another burst of activity right now and this looks important to solve.

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww
Status: Needs review » Needs work

Whoops, sorry. This no longer applies after I committed some other recent fixes. I'm going to try re-working this, and hopefully adding some test coverage, too.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
Issue tags: +Needs tests
StatusFileSize
new 3027696-5.patch1.12 KB

Here's the basic re-roll and a little code cleanup. Still probably wants some dedicated tests to demonstrate the problem and prove the solution.

dww’s picture

dww
we/he/they
commented
Issue tags: -Needs tests
StatusFileSize
new 3027696-6.test-only.patch1.34 KB
new 3027696-6.patch2.45 KB

Sorry, got pulled into other things for a bit there. Back at it. Here's trivial test coverage to show the bug. The test-only is the interdiff.

The last submitted patch, 6: 3027696-6.test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

Yee haw, those are the test failure results we're expecting:

1) Drupal\Tests\pathologic\Functional\PathologicTest::testPathologic
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-''">http://php-apache-jenkins-drupal8-contrib-patches-98931/subdirectory/http%3A//cdn.example.com/bar?test=external">'
+''">http://cdn.example.com/bar?test=external">'

  • dww committed dd804bc on 8.x-1.x 
    Issue #3027696 by dww, spleenboy: hook_pathologic_alter external option...

  • dww committed af6d2bb on 8.x-1.x 
    Issue #3027696 by dww: Follow-up fix to put the expected string first...
dww’s picture

dww
we/he/they
commented

Oh whoops, except the expected should always come first... I'll hotfix that.

dww’s picture

dww
we/he/they
commented
Assigned: dww » Unassigned
Status: Reviewed & tested by the community » Fixed

All fixed. Thanks, @spleenboy! And again, very sorry for the multi-year radio silence on your first contribution. I keep forgetting I co-"maintain" this module and almost never look at the issue queue. I'm on a little burst of activity now, and noticed this important fix. Hope you didn't get so discouraged that you didn't try to contribute anything else...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.