Uncaught exception in link formatter if a link field has malformed data

Changing the status to major as it leads to WSOD and it is also a system update issue.

@juancasantovenia may you provide some examples and steps to reproduce?

Mh, so we have some kind of non existing validation as part of saving some data with our API. While I agree that the actual site should not fail, we should also validate in the first place and ensure that the migrations are written in a way, that this cannot cause an issue.

Part of the issue is that the current code in LinkFormatter is doing a ternary. It shouldn't. Because the only valid returned values from getUrl() are InvalidArgumentException and a valid URL.

This site was an early adopter of D8. So it seems like perhaps the link validation either 1) wasn't fired in a migration because validation didn't exist outside of the FAPI or 2) it still doesn't exist. I'm also guessing that the source data was a text field that was used to generate a wrapper link around some image or text. Which isn't all that uncommon in D6. I've opened #2745779: Migration into Link field doesn't fire link validation to triage the migrate issue.

And now closing it duplicate to #2698083: D6->D8: Migrating links without leading slash leads to fatal error

Based on the findings in #2698083: D6->D8: Migrating links without leading slash leads to fatal error, we need to a fix here in link module. There are too many live D8 sites at this point. Sure, we should fix migrate moving forward. But we also need to wrap that URL building in a try/catch. Are there other options I'm missing? The site I'm dealing with has over 300 nodes that had malformed URLs. Causing significant issues with Search API and general navigation of the site.

+
+class LinkFormatterTest extends FieldKernelTestBase {

You are missing a @group statement :(

This is one possible solution.

@asolero, and I are working on triaging this issue at DrupalCon Vienna 2017 following the instructions in https://www.drupal.org/node/2474049. @ekl1773 is helping as our mentor.

Successfully applied https://www.drupal.org/files/issues/2745179-14.patch from #14 in 8.5.x and sent it for retesting.

Still unclear what "malformed data" is exactly in this context. How does a faulty link look like for example? How to actually reproduce this? Tagged for summary update.

Possibly related to #2233883: Link migration needs to convert source url into the appropriate route format for storage.

I have successfully applied the patch from #14 with Drupal 8.4.

In my case, I had import some data from a csv. This data was not in the best quality and some urls was not a valid url. Example: "http://Mo.-Do. 8:30-16:00; Fr. 8:30-12:00". This has lead in the "InvalidArgumentException" error.
With the patch, the problem was not triggered anymore.

+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -238,7 +238,13 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
-    $url = $item->getUrl() ?: Url::fromRoute('<none>');
...
+      $url = $item->getUrl();

Do we still need to handle the case that an empty URL is returned?

I think this is not the problem. Because, the buildUrl() function is not executed if the url field is empty. Even, if I remove the url value from the field in the database, this function is not executed.

Thank you for clarifying!

+++ b/core/modules/link/tests/src/Kernel/LinkFormatterTest.php
@@ -0,0 +1,132 @@
+   */
+  public function testBuildUrl() {
+    $malformedUrl = $this->randomString();
+    $entity = EntityTest::create();
+    $entity->set($this->fieldName, $malformedUrl);
+    $entity->save();
+    $this->renderEntityFields($entity);
+    $this->assertNoText($malformedUrl);
+    $this->assertText($this->label);
+

Could we assert the log message as well here?

I'm not sure if this was what was expected for the logging. I don't see any other tests in core that are doing this, so I had to guess this is what you mean.

The log message needs some love, as there is absolutely no information in there that will contribute to finding and fixing the root cause of the problem. Seeing as we catch the exception and convert it to a log message in the field formatter, can we add information about which entity and link field this is about, or perhaps even link to the edit page?

How about something like this? :)

+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -238,7 +238,19 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+      $url = Url::fromRoute('<none>');

Shouldn't we output a warning message as well?

As in a drupal_set_message type warning?

Yup. IIRC if the type is set to 'error', then only admins see it?

Setting a message is for the current user, regardless of message severity. What we can do is see if the user had edit permissions for the entity, and if they do, we display the error with instructions to fix it, and a link to the edit page?

A similar error occurs for the UriLinkFormatter "uri_link" formatter.

If a (base) field is defined as "uri" and the field contains for example "private://hello.txt" and you try to render that with "uri_link" formatter an exception of type InvalidArgumentException is thrown with the following message:

InvalidArgumentException: The URI 'private://hello.txt' is invalid. You must use a valid URI scheme. Use base: for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controlled by Drupal. in Drupal\Core\Utility\UnroutedUrlAssembler->assemble() (line 65 of core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php)

This can lead to fatal errors if an user accidentally selects this formatter for an uri field. For background, see #3043433: Error when trying to render field 'source' as uri.

I can open a new issue for this, but it seems at least closely related to this issue.

+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -238,7 +238,19 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
-    $url = $item->getUrl() ?: Url::fromRoute('<none>');

The getUrl method already guarantees a \Drupal\Core\Url object is returned, we should move exception handling there, and return the Url::fromRoute('<none>') instance when the exception is thrown

+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -238,7 +238,19 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+      watchdog_exception('link', $e, '%type: @message in %function (line %line of %file) when formatting field %field_label on %entity_type_label entity @entity_id (%entity_label).', $variables);

I dont think we should log any watchdog messages, we cant be expected to log messages whenever malformed data is loaded. Besides, the Url utility is already designed to throw exceptions in these cases, and we can safely ignore them. The existing line of code made the assumption that getUrl could return NULL, and falls back to the Url::fromRoute('<none>') instance, without logging any errors.

The getUrl method already guarantees a \Drupal\Core\Url object is returned, we should move exception handling there

Instead of doing that, updated \Drupal\link\Plugin\Field\FieldType\LinkItem::getUrl to document it potentially throwing exceptions. \Drupal\link\Plugin\Validation\Constraint\LinkTypeConstraintValidator::validate already catches this exception directly, and doesn't log or cause any other noise.

Major rewrite to tests from Kernel to Unit, as we're testing internals, not a functional site or the render system.

Test cases include expected exception, unexpected exception, valid URL.

Thanks, the fix makes sense.

+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -238,7 +238,12 @@ class LinkFormatter extends FormatterBase implements ContainerFactoryPluginInter
-    $url = $item->getUrl() ?: Url::fromRoute('<none>');
+    try {
+      $url = $item->getUrl();
+    }
+    catch (\InvalidArgumentException $e) {
+      $url = Url::fromRoute('<none>');
+    }

This change makes sense. I can't see how $item->getUrl() can ever return anything other than a Url object or throw an exception.

I think I'm +1 on the new approach but will wait a bit before committing to see what others think.

The change looks okay (though I haven't studied the tests), but this issue probably would require follow-ups for other field formatters that try to generate urls.

For example:

• Drupal\Core\Field\Plugin\Field\FieldFormatter\UriLinkFormatter
• Drupal\telephone\Plugin\Field\FieldFormatter\TelephoneLinkFormatter

I haven't checked TelephoneLinkFormatter, but if an uri is something like private://hello.txt, then trying to display that with the field formatter "uri_link" results into a fatal error. For background, see #3043433: Error when trying to render field 'source' as uri.

Quick re-roll now that #3059090: Deprecate \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException() has landed. Back to RTBC.

Goofed that up. Here's a better re-roll.

+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -238,7 +238,12 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+    catch (\InvalidArgumentException $e) {
+      $url = Url::fromRoute('<none>');
+    }

So I guess one thing we should be thinking about is should this result in a log message? @dpi argues that we shouldn't be logging something here but entities with invalid data are an error state that we should inform site owners about. How else are they going to know? At the very least we should have a comment here as to why we are not logging and eating the exception.

@alexpott
I would argue that the data does not have to be invalid, just not renderable by the selected format. In my opinion, something like private://hello.txt is a valid uri as it complies with the definition of a uri: https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Definition. It is just that the formatter can't handle that type of uri currently.

Perhaps we could add an option on the formatter for logging not renderable data? This way people can choose whether or not they want to catch data that the formatter can't handle.

So I guess one thing we should be thinking about is should this result in a log message?

I'm not particularly fond of logging messages to admin/system logs due to errors potentially caused by user invalid user input.

There is the existing LinkTypeConstraintValidator validator which eats the exception, which I'm guessing is a higher traffic code path than this widget, seemingly without any problem since at least 2016.

We are also experiencing the same issue on our website (D7 data migrated into D8, too)

For what it's worth, in my opinion committing this patch with or without log entry is better than not committing it. I opinion just a PHP comment about why we eat the exception is good enough, I do not think a log entry about malformed data is necessary. In fact, whenever you attempt to edit such malformed entity, Drupal validation API will not let you save it any more with an error message like "Web links entered must begin with ?, /, #, http:// or https://".

As for the patch itself #50, I find it impeccable :)

Logging is preferable with a handled exception so you are at least aware there is a problem and know where to look. If you are migrating data frequently (think feeds, APIs) into fields, this can happen often - so it isn't just when someone enters data into the Drupal admin form UI.

Here is one example of how it can get into Drupal:

field_my_link:
    - plugin: skip_on_empty
      method: process
      source: my_url
    - plugin: build_link_array
    - plugin: sub_process
      process:
        uri: uri
        title: title

Where 'uri' is bad, only its stored into the field. Then, when rendering, you get:

The website encountered an unexpected error. Please try again later.InvalidArgumentException: The URI 'XYZ' is invalid. You must use a valid URI scheme. in Drupal\Core\Url::fromUri() (line 290 of core/lib/Drupal/Core/Url.php)

.

re #60: one suggestion in this specific case is to turn on entity validation on the destination entity plugin.

destination:
  plugin: 'entity:node'
  validate: true

Will note that in the future. Might be hard to go that route though since we are over 100,000 items.

The patch in #50 did apply though in 9.2.x, and fixed the issue I mentioned in #60. I can't replicate the error from that anymore.

I'm wondering if there's appetite to commit the patch as is and move the logging discussion into a follow-up? I can see there being a bit of churn to get that right and this has been sitting untouched for quite a while now.

Setting to NR to see if anyone else agrees with #67

Been 5 months. @acbramley lets do what you suggested. Moving to NW for the follow up.

Created #3348020: Add logging to LinkFormatter and LinkWidget when the URL is invalid

Moving to NW as the patch matches the issue summary.
Test fails and shows the issue with error InvalidArgumentException
Think this is good. Patch #50 applies to 10.1 cleanly also.

In which case, let's add a comment referring to the followup (e.g. @todo) as per #51

Added todo.

Todo was added.

Just noting there is a similar issue with the Link Widget for users who don't have "link to any page" permissions: #3340154: Link-widget throws exception when rebuilding a form with an invalid uri

Committed to 10.1.x and backported to 10.0.x
Ran the new test locally on 9.5.x (PHP 7.3)

⏳️ Bootstrapped tests in 0 seconds.
🐘 PHP Version 7.3.33-10+ubuntu22.04.1+deb.sury.org+1.
💧 Drupal Version 9.5.8-dev.
🗄️  Database engine mysql.
PHPUnit 9.6.6 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\link\Unit\LinkFormatterTest
...                                                                 3 / 3 (100%)

Time: 00:00.031, Memory: 12.00 MB

OK (3 tests, 12 assertions)

A similar problem exists with the MailToFormatter where an invalid email address is present, eg '000' - fromUri throws an InvalidArgumentException resulting in WSOD. Although the UI prevents this, our system ingests data from other systems which hasn't been validated. Is it worth raising a separate issue for that? Or is it worth addressing at all, since really the data should be valid in the first place?