8.x-1.0-alpha3 breaks images in multi-language sites.

Hrm, bummer. I specifically did *not* commit #2418369: Internal URL handling (language prefixes, base://, ...) since it seemed to be not totally bullet-proof for country codes. #2993935: Support for srcset attribute seems unlikely to have caused this. #2692641: Convert href to the aliased URL if possible might be the culprit, but that seems a bit unlikely, too. :(

Can you provide the resulting <img> source from the page views for the two cases? And the raw value in the text prior to the filter running?

Do you really need #2718473: Dynamic routes in image styles break under pathologic, instead? Did you used to apply any pathologic patches and are no longer applying them after the update?

We need more info here on how to reproduce this before anyone can come up with a fix.

Thanks/sorry,
-Derek

Thanks @jedgar1mx. Can you also provide what the source from the body field looks like? Or whatever text is being filtered to produce these <img> tags?

Also, links that are used as absolute, like https://example.com/en/node/1, becomes to https://example.com/en/en/node/1 with double language prefix.

Ugh, bummer. I now have almost no confidence in the existing test suite. All of this should have been caught by tests, but that clearly didn't happen. I suspect #2692641: Convert href to the aliased URL if possible is the culprit. This patch reverts that change. Does this fix your sites?

Ran into the same issue. Tested #9 and it resolves it.

#9 solves issue.

On D93.13 + 8.x-1.0-alpha3 , paths images (PNG, not SVG) are broken if I enabled the input filter.
Applying the patch works.

I came to this issue because I had the same error. However, I have approached it in a different way:

/**
 * Implements hook_pathologic_alter().
 */
function hook_pathologic_alter(&$url_params, $parts, $settings) {
  if ($settings['is_file'] ?? false) {
    $url_params['options']['use_original'] = true;
  }
}

Can't we do the same in the _pathologic_replace function? Or am I missing some side effect?

Patch #9 working fine with me

My relative and absolute links added through text editor started showing the double language prefix when updating to 8.x-1.0-alpha3. Patch in #9 fixed the issue and should be merged and released as a new version as soon as possible.

I tried the patch #9 but it removed language prefix from my links in paragraphs.

By the way, I was facing a similar problem with PDF files and I solved it using the dedicated hook :

/**
 * Implements hook_pathologic_alter().
 */
function MYMODULE_pathologic_alter(&$url_params, $parts, $settings) {
  // Prevent pathologic to add language prefix to files.
  if (preg_match('/^sites\/default\/files/', $url_params['path'])) {
    $url_params['options']['path_processing'] = FALSE;
  }
}

Hope it could help !

Attempting to sum up the discussion up to this point:

1. The basic problem is that Pathologic now adds a language prefix to links to files on sites where Drupal's multilingual features are enabled.
2. The patch in #9 has been reported to fix the issue for a number of folks (10, 12, 13, 16).
3. There is currently no test coverage that verifies the fix.
4. One comment (#8) reported the same problem on absolute links ("Also, links that are used as absolute, like https://example.com/en/node/1, becomes to https://example.com/en/en/node/1 with double language prefix.")
5. Another comment *#17) reported that the patch in #9 actually removed language prefixes from links within paragraphs (assuming this isn't referring to the Paragraphs module).

Based on the above, I will work on the following:
1. Create test coverage that establishes the expected + actual output of linked files, linked absolute paths, and linked relative paths when multilingual features are enabled.
2. Provide that as a standalone patch to demonstrate current failure.
3. Add a separate patch that includes the refactor from #9 and establish whether or that that handles all 3 scenarios correctly.

Okay, test coverage!

The test scaffolding was largely derived from Drupal core's FileOnTranslatedEntityTest and PathLanguageTest tests. I then added logic to enable Pathologic on a text format. The test creates a node with a link (to a file path, to another node) and then creates a translation of that node. It verifies that the link on the translated node does not have the language prefix.

There are two patches:

• 3270030-language-19.testonly.patch: this only includes the test coverage. We should expect to see a failure when run.
• 3270030-language-19.testandfix.patch: this has the test coverage, plus the fix from #9. We should expect to see this pass.

The patches apply cleanly to both the 8.x-1.x and 2.0.x branches, and upon review & approval, should be merged into both branches.

It looks like the syntax of the test doesn't work in the context of Drupal 9.5.x (I constructed it using 10.x). I'd like to run the same patches on the 2.0.x branch to verify that I'm not getting a different result locally than the test runner. Assuming not, then I'll look into different test logic that works with Drupal 9.5.x.

Looks like my local testing environment didn't care that drupalGet() didn't start with a /, whereas testbot does? (Tipped off from https://www.drupal.org/project/commerce/issues/2855587#comment-11956902).

Trying one more time...!

There are two patches:

• 3270030-language-26.testonly.patch: this only includes the test coverage. We should expect to see a failure when run.
• 3270030-language-26.testandfix.patch: this has the test coverage, plus the fix from #9. We should expect to see this pass.

The patches apply cleanly to both the 8.x-1.x and 2.0.x branches, and upon review & approval, should be merged into both branches.

Thanks for diving into this mess! 😅 See also #3157085: Convert tests/src/Functional/PathologicTest.php to a Kernel test, which I'd love to move forward as we try to get the test coverage here into better shape...

I don't have time right now to dig deeper into what you're doing here, but hopefully soon.

Cheers,
-Derek

Alright, inspecting the artifacts on the d.o test runner, I see that it's adding a subdirectory/ prefix to the URL; this is not specific to Pathologic's processing of relative URLs. With that in mind, I did one more refactor to the test that explicitly checks that the href we want (/sites/default/file.png) is on the page, and also explicitly checks that the language prefix ( (/fr/sites/default/file.png) is not.

Sorry for all the noise, everyone!

Thanks for keeping this moving! Yeah, the testbot uses a subdir, partly to flesh out bugs that are hidden on sites that don't use them. 😉

So the only "fix" to get the test working is to revert the 2-line change from #2692641: Convert href to the aliased URL if possible, huh? That's all patch #9 is. Seems we should probably re-open that issue. I should have required test coverage over there before we committed it, since I'm sure reverting will piss off the non-multilingual folks that were so excited about that issue. 😬

Meanwhile, the full patch says "6.66 KB", so we can't possibly commit that, especially not on Ash Wednesday! 😂

Since this is my first time reviewing your test writing, here's a close review as I see things:

1. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +  protected $defaultTheme = 'olivero';
   

   Do we care that the markup is from olivero for some reason? I'd much rather test with stark if possible. It's much more generic, less fragile for testing, is compatible across way more branches, probably makes the tests a bit faster, etc.

2. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +    $this->drupalGet('/admin/config/regional/language/add');
   +    $this->submitForm($edit, 'Add language');
   

   This is slow and expensive. Especially inside a setUp() method that's called over and over again in a Functional test for each test*() method. I'd much rather do something like this:

       ConfigurableLanguage::createFromLangcode('fr')->save();
   
       // In order to reflect the changes for a multilingual site in the container
       // we have to rebuild it.
       $this->rebuildContainer();
   

3. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +    $edit = ['language_interface[enabled][language-url]' => 1];
   +    $this->drupalGet('/admin/config/regional/language/detection');
   +    $this->submitForm($edit, 'Save settings');
   

   Ditto here. I think this can be something like:

   \Drupal::configFactory()->getEditable('language.negotiation')
         ->set('url.prefixes.fr', 'fr')
         ->save();
   

4. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +    $this->drupalGet('/admin/config/regional/content-language');
   +    $this->submitForm($edit, 'Save configuration');
   

   Probably this, too, although a quick spot check, and for some reason, most core tests I just looked at which deal with translation do this via the UI... 🤔

5. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +    // Enable Pathologic on a text format.
   +    $this->drupalGet('/admin/config/content/formats/manage/plain_text');
   +    // Select pathologic option.
   +    $this->assertSession()->pageTextContains('Correct URLs with Pathologic');
   +    $this->assertSession()->checkboxNotChecked('edit-filters-filter-pathologic-status');
   +    $this->submitForm([
   +      'filters[filter_html_escape][status]' => FALSE,
   +      'filters[filter_pathologic][status]' => '1',
   +      'filters[filter_pathologic][settings][settings_source]' => 'local',
   +      'filters[filter_pathologic][settings][local_settings][protocol_style]' => 'path',
   +    ], t('Save configuration'));
   

   And for all of this, should we move _pathologic_build_format() out of PathologicTest.php, turn it into a test trait, and share it so we can easily make filters that do what we want instead of having to click it together in the UI in setUp() like this?

6. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +  public function testLinkedNode() {
   

   We can add the : void for this.

7. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +    $this->drupalGet('/node/add/page');
   +    $node_to_reference = $this->drupalCreateNode([
   ...
   +    $this->drupalGet('/node/add/page');
   +    $default_language_node = $this->drupalCreateNode([
   

   If you're using drupalCreateNode() (yay!) you don't need the drupalGet().

8. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +    // Create a translation of the node, same content.
   +    $this->drupalGet('/node/' . $default_language_node->id() . '/translations/add/en/fr');
   +    $this->submitForm([], 'Save');
   

   All this can be:

   $default_language_node->addTranslation('fr')->save();
   

   Or, if you want to modify stuff in the translation, you can just do something like:

   $default_language_node->addTranslation('fr', ['title' => 'French translation', 'status' => NodeInterface::PUBLISHED])->save();
   

9. +++ b/tests/src/Functional/PathologicLanguageTest.php
   @@ -0,0 +1,153 @@
   +   * Tests that Pathologic does not prefix URLs of file types.
   

   Perhaps to a fault, but as you can see, I get really hung up on test performance, especially in Functional tests (even more so in FunctionalJavascript). 😅 Given how much CO2 we burn for all the computing cycles for all the automated tests that run, and for our own lives while doing test-driven dev, I'd rather our test suite be as fast as possible.

   So I wonder if we can merge these two tests, create all the links we need to test in the smallest number of nodes, fetch the output once, and make as many assertions as possible about the links on the page? Basically, think of every drupalGet() or submitForm() in a Functional test as a $50,000 expense over time. 😅 I try to minimize those...

10. +++ b/tests/src/Functional/PathologicLanguageTest.php
    

    Reflecting on my last point -- does this need to be a Functional test at all? Can't we generate the content, run the filter, and check the output in a Kernel test? Why do we need a test browser for this at all?

Phew! Ended up being a long list. But overall, this is great work, and I love that you're providing solid test coverage of what's happening in here! 💗 Curious to hear your thoughts.

Thanks again!
-Derek

Thanks, so much, for the thoughtful and thorough feedback, Derek! Regarding making tests run efficiently, you have a kindred spirit here. I've added tweaks to our team's Drupal distribution test suite to minimize the number of setups it does and use a core patch that allows FunctionalJS setups from an existing database rather than a fresh install. Balanced with that, I also tend to approach writing tests as self-documenting 'steps to reproduce' with the aim of being more accessible to other developers.

There were a few reasons I was thinking this particular test should be Functional rather than Kernel, and UI-based:

1. As an integration test of Drupal's multilingual functionality, I think it makes more sense to put this through the Drupal pipeline to the rendered page. This allows us to orchestrate Drupal's text filter rendering with Drupal's content translation in the same way a site builder would, and in the future would allow testing integration with other things (such as other text filters designed to modify links) more directly, rather than mocking them.
2. Because it involves Drupal's multilingual functionality, which fewer contributors are familiar with, it will be easier for other contributors to understand what is going on in the test if more of the multilingual configuration actions step through the UI.
3. From a purely technical perspective, it wasn't immediately clear to me how a Kernel test would do the equivalent of visiting /node/1 vs. /fr/node/1.

Other suggestions: 100%!

- Stark instead of Olivero (I'd started with Stark, then switched to debug HTML output, then forgot to switch back!)
- One test instead of two (I'd done two for DX, but we can surely maintain good DX in one test)
- Reduce the number of nodes we need to create (follows from the above)

Would it make more sense to create follow-up task for relocating _pathologic_build_format() into a test trait (or better: FunctionalTestBase method?), since it would require differences between the 8.x-1.x and 2.0.x patches? Or should we do that just for the 2.0.x branch?

Adding to what dww said in #32 about core tests for translation using the UI, I took a look at core's tests for testing translated output (mostly in the content_translation module). I didn't find a good model of a core Kernel test for content translation. That's not to say it can't be done, and I'm not opposed to using Kernel tests; just that we don't have a good paradigm to work from.

But taking it from another perspective -- looking at how we are approaching the Kernel tests in #3157085: Convert tests/src/Functional/PathologicTest.php to a Kernel test -- if the test coverage we're trying to provide is to confirm that the Pathologic text filter is *not* adding language prefixes to file paths or bare node paths, and since check_markup() takes an optional language code parameter, would it be sufficient test coverage to do something like the following (non-tested-code)?

    $this->assertSame(
      '<a href="/sites/default/files/test.png">bar</a>',
      check_markup('<a href="/sites/default/files/test.png">bar</a>', $format_id, 'fr'),
      t('Language codes are not added')
    );

Update: the maintainers discussed this, and given the value of providing a Drupal 10-compatible release sooner (see #3343541: Plan for new Pathologic releases), one that includes reverting the change that led to this issue in the first place, we have decided to:

1. Merge this issue, with its working Functional tests now.
2. Cut a Drupal 10-compatible release from the 2.0.x branch
3. Finish work on #3157085: Convert tests/src/Functional/PathologicTest.php to a Kernel test to convert other existing tests to Kernel-based.
4. Create a follow-up issue to convert the Functional test added here to a Kernel-based test.

Yup, I'm on it. Just found some other test badness that we need to fix, first: #3353557: PathologicUITest should not be testing the Internet. As soon as that's resolved, I'm going to proceed here.

Thanks!
-Derek

Committed to 2.0.x and cherry-picked to 8.x-1.x. Pushed to both branches. Thanks!

p.s. The follow-up is a child of this issue, but linking here for more visibility:
#3353562: Convert PathologicLanguageTest to a Kernel test