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

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.

The last submitted patch, 20: 3270030-language-19.testonly.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 20: 3270030-language-19.testandfix.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

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.

The last submitted patch, 23: 3270030-language-19.testonly.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 23: 3270030-language-19.testandfix.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

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.

The last submitted patch, 26: 3270030-language-26.testonly.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 26: 3270030-language-26.testandfix.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Thanks for diving into this mess! 😅 See also 📌 Convert tests/src/Functional/PathologicTest.php to a Kernel test Fixed , 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!

The last submitted patch, 30: 3270030-language-30.testonly.patch, failed testing. View results →

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 ✨ Convert href to the aliased url if possible Needs work , 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 📌 Convert tests/src/Functional/PathologicTest.php to a Kernel test Fixed -- 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 🌱 Plan for new Pathologic releases Active ), 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 📌 Convert tests/src/Functional/PathologicTest.php to a Kernel test Fixed 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: 🐛 PathologicUITest should not be testing the Internet Fixed . 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:
📌 Convert PathologicLanguageTest to a Kernel test Fixed

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