Handle NULL URL options in LinkFormatter::buildUrl()

I'm wondering whether we can prevent $item->option being NULL in the first place?

Does it happen for all 3 Link field configs? Internal, External, and Internal or External?

This issue may be related (or maybe a duplicate) to #2802403: Combination of language negotiation and path aliasing can cause a corrupted route cache, 404s

I think no question, we should fix this, but understanding where this is coming from might result into an underlying bug we should fix as well / instead. Hiding notices, can have small negative impacts.

We have the same issue, also in a multi-level paragraphs context, on a monolingual french site.

The patch could be simplified, I think:

   $options = $item->options;
   if (is_null($options)) {
     // Avoid error if $options is not set.
     $options = [];
   }

Could be written more simply as just:

$options = (array) $item->options;

Rerolled. Testing against 8.4.x since this is a just a bug which could still go into 8.4.x.

Rerolled on today's 8.5.x HEAD.

We should expand the test coverage in \Drupal\Tests\link\Functional\LinkFieldTest::testLinkFormatter.

Thank you all for the patch.
I'm using Drupal 8.4.5 and the patch #11 worked.

I was having this issue with certain nodes AND on certain pages number from blocks generated with views.
For example:

• node/1584 throw error, god does only know the reason because it was the same as many others that did work
• news/politics?page=4throw error also, but previous pages don't, only from #4 -_-

I tried modifying testLinkFormatter to run

        entity_get_display('entity_test', 'entity_test', 'full')
          ->setComponent($field_name, ['settings' => NULL])
          ->save();

but it is expected to be an array elsewhere!

array_intersect_key(): Argument #1 is not an array
/core/lib/Drupal/Core/Field/FormatterPluginManager.php:154
/core/lib/Drupal/Core/Entity/EntityDisplayBase.php:371
/core/modules/link/tests/src/Functional/LinkFieldTest.php:430

I do also wonder if fixing this in LinkFormatter is correct or if we should enforce it higher up the chain, e.g. PluginSettingsBase:;getSettings() claims to return an array according to the interface docs, so why shouldn't it always do that?

Hi,

I've tracked this to a field on a node type. Not related to paragraphs or translations.
Nodes are imported through custom code, so fields are initialized with the following statements:

// ... Stuff...
// ... other fields.
$node->field_news_url->uri = 'https://example.com/path0';
// ... other fields.
$node->save();
// ... Stuff...

This works correctly* (no exceptions are thrown) to write the URI but as options and title are not required (on API level) and apparently no defaults get stored in DB as well, once it gets read from there it's plain wrong, as sensible defaults are not applied at that point on field level.

Here is an extract from my field's table. URLs are sanitized...

bundle	deleted	entity_id	revision_id	langcode	delta	field_news_url_uri	field_news_url_title	field_news_url_options
news	0	15	92	en	0	https://example.com/path0	NULL	N;
news	0	16	93	en	0	https://example.com/path1	NULL	N;
news	0	17	94	en	0	https://example.com/?query=0	NULL	N;

The items are already incomplete after this call.

$field = \Drupal::entityTypeManager()->getStorage('node')->load(16)->field_news_url[0];
var_export([$field->uri, $field->title, $field->options]);
// output: array( 'https://example.com/path1', NULL, NULL )

A correct fix would be:
Option 1: (I like this one better)
To fix the save behavior on the field, so defaults are applied correctly before data is saved.
Have an update hook to resolve existing data mutations in DB, so invalid data is fixed.

Option 2:
Have the defaults applied correctly on field read, so all dependent code (widgets, formatters, etc.) will HAVE valid values.

This is based on the patch from #11.
Here is a patch that adds the requested test tweaks in #12.

Another attempt at a fix.

Changing status...

I did not test the patch, but I would just like to add for anyone that might have similar issues as me:
When importing a D7 field that included the twitter username, which I had to migrate to a link field type, I passed an empty string to "options", ended up with this error, and fixing it like so (inside a process plugin):

 public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
...
 return [
      'uri'     => 'http://www.twitter.com/' . $username,
      'title'   => $username,
     'options' => 'a:0:{}', // Add this to fix error
    ];
...

Just wanted to drop a note, even though fixing this automatically on migrations would probably call for a separate issue on the migrate module?

It works great. +1

May I urge you to add that (array) finally? The same error crops up with all updates again and again.

Woohoo! Patch #19 works great for me. Thanks! :)

Patch #19 worked for me as well. Thanks

As per #23, #24, #26 and #27 moving to RTBC.

This looks like corrupted data to me, could we look again at 'option 1' from #16?

It would be ideal if we could find the actual source (option 1 #16), but it won't fix the issues for all the sites having been corrupted in the meantime. So even though the complete fix should be at the error root, I think it should still include at least the one-word fault-resilience patch. (the (array) cast) which has about zero cost and make core more resilient.

Add that (array) from patch #19 right now! Today! This is insane. I spent nearly an hour to find out what went wrong with a Drupal update on a site. And when I finally found this thread, I was shocked to see that I had the same problem about a year ago, and had to come here to urge you to add that single cast, absolutely free, no performance penalties, nothing. It was the same darn error back then, and of course, long forgotten, and it appears again and again, about a year later. I don't care about the reasoning that it shouldn't come up, yes, sure, there is some strange data somewhere but not all sites are re-made every six months from scratch, some are ported from version to version, have lots of stored data, and so on. It's really unacceptable to leave out such a simple solution that sanitizes even a situation that you don't think will crop up but nonetheless, it does.

Add that cast now.

#31. Another Drupal release comes, 8.8, and again, and again, and again, upgrade kills the site to a WSOD because you refuse to add that single obvious cast. Why?

@djg_tram
You can use composer patches and have the patch applied automatically on updates until this is resolved.
This should fix the WSODs caused by this issue, as well as you will know sooner if the existing patch does not apply on the latest version.

In the meantime, I am a HUGE +1 on the two comments from #30 and #31.

Is anyone sitting on an 8.8.x-compatible patch for this? Can you upload it? Otherwise, I will do so in a bit (based on #19, which we were using before).

This is for 8.8, but figured I'd run 8.9 tests as well in case anyone needs that. This shouldn't be RTBC'd even if the tests pass since it's not actually against dev.

Back to Needs work so someone can actually roll a patch against 8.9.x-dev

Attached is the patch re-rolled against 8.9-dev.

+++ b/core/lib/Drupal/Core/Field/FormatterPluginManager.php
@@ -150,8 +150,9 @@ public function prepareConfiguration($field_type, array $configuration) {
+    $settings = isset($configuration['settings']) ? $configuration['settings'] : [];

For PHP7+ this can just be:

$settings = $configuration['settings'] ?? []

(null coalesce operator)

Otherwise, patch seems reasonable. Didn't super-closely review nor test manually, so not RTBCing.

Sure. Updated the patch to use the null coalesce operator.

Looks good to me. Removing the special casing in the formatter indicates this is on the right path.

+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -241,7 +241,7 @@ protected function buildUrl(LinkItemInterface $item) {
-    $options = $item->options;
+    $options = (array) $item->options;

If i remove this change and run the test it passes - so this bit of the change doesn't have test coverage. However if I remove the other part of the fix and leave this in the test fails - so is this bit actually necessary? And if so can we prove it by adding a test. I think it is because it's the bit that sorts out the NULL options whereas the other stuff is for the NULL settings.

Also now that we have PHP7 rather than an array cast here we could do $item->options ?? []; because that's more specific to the NULL case.

Here this patch might fix comment #42

@ravi.shankar that addresses the last part but not the

If i remove this change and run the test it passes - so this bit of the change doesn't have test coverage. However if I remove the other part of the fix and leave this in the test fails - so is this bit actually necessary? And if so can we prove it by adding a test. I think it is because it's the bit that sorts out the NULL options whereas the other stuff is for the NULL settings.

part

Back to needs work to address #44.

Crediting folks from #3174350: Unsupported operand types in LinkFormatter->buildUrl() which I marked as a duplicate

According to reports in #3174350: Unsupported operand types in LinkFormatter->buildUrl() this causes a WSOD on php7.4, so raising this to major

Using Drupal 8.9 and php 7.3. I was getting a WSOD when trying to edit a translated version of a node that uses paragraphs. Applied patch in #43 and seems to be working fine. Thanks!

I ran into a weird issue where the options was set to s:6:"a:0:{}";, which then returned a string. I was able to make a minor change to the patch #43 which adds a check to make sure it's an array.

#60 worked for me in Drupal 9.3.12 with PHP 8.0.

@jsutta Would you please explain how you tested? Thanks.

Patch #60 worked for my use-case. We had a migration from D7 to 9.3.14. There was a link field to migrate and the migration snippet looks like this:

field_info_link:
    plugin: sub_process
    source: field_f_link
    process:
      uri: url
      title: title
      options: attributes

All of the migrated nodes showed this when we're trying to view them (although editing was possible) :

The website encountered an unexpected error. Please try again later.
Error: Unsupported operand types in Drupal\link\Plugin\Field\FieldFormatter\LinkFormatter->buildUrl() (line 244 of core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php).
Drupal\link\Plugin\Field\FieldFormatter\LinkFormatter->buildUrl(Object) (Line: 178)
Drupal\link\Plugin\Field\FieldFormatter\LinkFormatter->viewElements(Object, 'de') (Line: 89)
Drupal\Core\Field\FormatterBase->view(Object, 'de') (Line: 263)
Drupal\Core\Entity\Entity\EntityViewDisplay->buildMultiple(Array) (Line: 340)
Drupal\Core\Entity\EntityViewBuilder->buildComponents(Array, Array, Array, 'full') (Line: 24)
Drupal\node\NodeViewBuilder->buildComponents(Array, Array, Array, 'full') (Line: 282)
Drupal\Core\Entity\EntityViewBuilder->buildMultiple(Array) (Line: 239)
Drupal\Core\Entity\EntityViewBuilder->build(Array)
call_user_func_array(Array, Array) (Line: 101)
Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725', 'exception', 'Drupal\Core\Render\Element\RenderCallbackInterface') (Line: 772)
Drupal\Core\Render\Renderer->doCallback('#pre_render', Array, Array) (Line: 363)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 201)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 241)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 242)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 132)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 142)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.view') (Line: 163)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 708)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

After applying the patch, everything worked as expected.

This bug caused a WSOD on a production server (Drupal v9.4.5) when I added a link field to a paragraph (v1.15.0) that then got translated across multiple languages via Lingotek. A workaround was to add the language prefix to each translated URL, but patch #60 fixed it for me.

Edit: I think Lingotek may be corrupting the link when translating because when I enter the English URL on a French paragraph it works fine without the patch.

Added reroll of patch #60 on Drupal 9.5.x, and added reroll diff.

Also added interdiff between patch #43 and #patch 60.

I think it's not ready for RTBC as per comment #44, so back to needs work.

Fixed Drupal CS issue of patch #66.

Added Re-roll patch #60 on Drupal 9.5.x

(Naughty bug removed my credit for the original patches)

Found a related issue in the Drupal 8+ migration (source) module:

#3184165: serialized values becomes strings

Restoring issue summary, tags and related issue which was removed in #68.

Re-roll patch needed for 11.x.

Re-roll patch created for 11.x.

I added the Needs issue summary update tag because it would help to understand remaining work and the proposed resolution specifically the test coverage expansion in the issue summary rather in the comment chain.

I am working with @ompiepy at MidCamp 2024 and we're going to look at this issue to try to move it forward. Possibly transitioning to a merge request after updating the issue summary.

I am looking into the issues to get the summary update and then start working for the merge request.

Update: I am done for the day. But, I will try to update and love to help. Slack: Om Prakash Sharma

Did not review

Issue was tagged for issue summary update which appears to still be needed.

To handle the error when $options is NULL in buildUrl(), you can add a simple check to ensure that $options is always an array.
Something like this

$options = is_array($options) ? $options : [];

The patch we suggested seven years ago (!), simply casting to array, was already capable of accomplishing this. :-)

I have long moved on since and no longer bitten by this problem but this one is really ridiculous, I have to say. A very straightforward modification that obviously doesn't even require testing, it cannot cause any problems or regresssion by definition, and nobody ever bothered to accept it (or even argue against it). Must be one of the record setters in Drupalland. :-)

In response to #3 and #76:

The options of an URL object are documented as optional, which means NULL is a valid value.

So it does not really matter where the underlying problem is coming from. The problem is that the code blindly assumes that $options is a mandatory array, which does not comply with the spec. So any code in core that assumes that options is array, should be rewritten to be null-safe.

I got into this ticket due LinkGenerator, which has exactly the same incorrect not-null assumptions (line 93 and line 154)

So IMHO the underlying problem is not that somwhere the options aren't set, the underlying problem is that this code is not following the specification. I've added this to the issue summary.

patch for 11.2 without tests

> The options of an URL object are documented as optional, which means NULL is a valid value.

It doesn't mean NULL is valid, because the default $options is an empty array. Eventually we will be adding types and this argument will be typed as an array.

This means it is the caller's responsibility to ensure an array is passed in. In the LinkFormatter case, the underlying options property in LinkItem is a Map, which is stored either as an array of values or NULL. Therefore I think the fix should be in LinkFormatter::buildUrl():

    $options = $item->options ?? [];

I did a deep dive on this issue and discovered problems with the assumptions and the work that has been done so far in this issue. I'm fortunate to be debugging this with later versions of PHP that provide better error messages. The issue summary has been properly updated with detailed descriptions of the problems at hand.

The first important thing to understand is that LinkFormatter is not receiving NULL as the options value. It's actually FALSE. As far as I can tell it has always been FALSE. In fact, none of the above patches that explicitly treated the value as being NULL, i.e. corrected it with the NULL coalescing operator, were reported as working. The only "working" patches are ones that just happen to correct the FALSE case.

The second important thing to note is that this really isn't an issue with the formatter. Any attempt to correct the problem in it will only cause the unserialization warnings to become more apparent. Then eventually unserialize() will start throwing errors instead of warnings and we'll be back to having WSoDs again. The inherent problem is lower-level than that. We allow a field column to be NULL, but the DB layer always tries to unserialize the value. I don't know that this is even the DB layer's fault. It may be the field's for not requiring it. I have to solicit opinions about how it should be fixed.

Postponing on #3300404: Handle nullable serialized field columns which should correct the unserialization problem. If the proposed fix for that one is implemented, then the options value will actually be NULL and we can proceed from there.

This is unblocked now.

Pretty straight forward fix

1) Drupal\Tests\link\Kernel\LinkFormatterDisplayTest::testNullLinkOptions
TypeError: Unsupported operand types: null + array
/builds/issue/drupal-2871217/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php:249
/builds/issue/drupal-2871217/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php:180
/builds/issue/drupal-2871217/core/lib/Drupal/Core/Field/FormatterBase.php:91
/builds/issue/drupal-2871217/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php:275
/builds/issue/drupal-2871217/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php:233
/builds/issue/drupal-2871217/core/lib/Drupal/Core/Entity/EntityViewBuilder.php:462
/builds/issue/drupal-2871217/core/lib/Drupal/Core/Field/FieldItemList.php:243
/builds/issue/drupal-2871217/core/modules/link/tests/src/Kernel/LinkFormatterDisplayTest.php:279
ERRORS!
Tests: 2, Assertions: 206, Errors: 1, PHPUnit Deprecations: 3.
Exiting with EXIT_CODE=2

Shows test coverage, good work keeping it to a kernel test btw.

Thanks @dcam for digging into the actual issue here and figuring out the correct fix.

@djg_tram you repeatedly demanded that an incorrect fix that wouldn't have solved the bug for many cases be applied, and by adding pointless noise to the issue personally contributed to it taking longer to fix than it should have. Shouting at everyone else to fix it faster does not actually help.

Committed/pushed to 11.x, thanks!