Array merge failure on line 153 of core/lib/Drupal/Core/Utility/LinkGenerator.php

Yeah I think when we fix something here we might should have a test for it.

I am not even sure why we need a placeholder for 'href' in the array, as it is explicitly set in both branches of the subsequent if() statement anyway.

The actual problem here seems to be that $variables['options']['attributes'] is something other than an array in this case.

I do think my solution resolves for this though, as array_merge handles the situation gracefully while "+" errors out due to mismatched types.

That seems to be a workaround to be honest. If someone passes in crap data it is totally fine to have a notice, IMHO.

Up until line 149 the $variables['options']['attributes'] is guaranteed to be an array because it was explicitly set above. But on line 149 there is a link alter called which presumably the $variables['options']['attributes'] could be set to anything. I think that making the assumption that 3rd party developers have not altered the data "incorrectly" is maybe a bad idea, but it could also be difficult to react to all possibilities. Every call to any item in $variables could return an error if it was unset. I can't imagine that this is the first time Drupal has had to deal with this issue. Is there a standard way of handling this?

Just to be clear, dawehner, you say "it is totally fine to have a notice". I'm getting a fatal error. I agree, notices and warnings for meh data makes sense.

If the developer made a mistake IMHO telling the, is absolutely the right thing to do.

If you care about those level of things on production you can turn it off, but the more we tell developers the better.
For this particular issue it might make sense to use an assert() here.

Well fair, I think we should add an asset which then informers the developer and stops early.

Just moving ...

What is the value of $variables['options']['attributes'] when it is not an array? What are the other properties of the link that is being generated? If this is caused by a migration from D6 then this suggests a bug in the migration code that should be fixed at source. Is it possible to trigger this from a fresh install of D8 without writing custom code?

I hit this exact same issue and tracked it to an issue with the migrating of link fields from Drupal 6 to Drupal 8.

I used the migrate_plus /upgrade to migrate a D6 site into D8. One of the content types that was migrated had a link field. Post-migration, in Drupal 8, when the node is rendered, I used my debugger to find that the value of $variables['options']['attributes'] was "false" - clearly not an array. This is what leads to the error on line 152:

$attributes = array('href' => '') + $variables['options']['attributes'];

I'm guessing that there is an issue with link migrations where if there are no attributes on the source site, the default value is "false" rather than an empty array.

As further proof, if I edit and save the same node in D8, the error goes away (I'm assuming that D8 correctly sets it to an empty array).

-mike

I think that line of code is wrong even if it is not checking if the $variables['options']['attributes'] is an array.

Accord with the Array operators.:

The + operator returns the right-hand array appended to the left-hand array; for keys that exist in both arrays, the elements from the left-hand array will be used, and the matching elements from the right-hand array will be ignored.

If we have something like this:

$variables['options']['attributes'] =  [
  'attribute1' =>  'value',
  'href' => 'http://www.google.com.mx',
 'attribute2' => 'value 2'
];

Then the $attributes variable will have these values:

$attributes =  [
  'attribute1' =>  'value',
  'href' => '',
 'attribute2' => 'value 2'
];

So, it basically is overwritting the href value of the original attributes with an empty string, so indeed this needs to be an array_merge or switch the order of the params.

You can see the proof here: https://3v4l.org/W6J8f

I want to work on this, so I will assign the ticket to me.

UPDATE: Ignore this comment, the href value is get it from the $url object a few lines below, that line just make sure to that key exists in the array.

I studied the code and these are my findings:

Let's assume to we havefalse as the value of $variables['options']['attributes'];

With our current code we get this:

I think this is not the ideal behaviour, because this kill our site.

This is what we get using the patch of [#2]:

We still get warnings but the links are render fine, so while this is better (because we can just hide the errors in our production site) I think this is not ideal either.

My proposal is just to get rid of the the href place holder:

$attributes = $variables['options']['attributes'];

The array('href' => '') is not even necessary because below that attribute is defined:

 if ($url->isExternal()) {
      $generated_link = new GeneratedLink();
      $attributes['href'] = $url->toString(FALSE);
    }
    else {
      $generated_url = $url->toString(TRUE);
      $generated_link = GeneratedLink::createFromObject($generated_url);
      // The result of the URL generator is a plain-text URL to use as the href
      // attribute, and it is escaped by \Drupal\Core\Template\Attribute.
      $attributes['href'] = $generated_url->getGeneratedUrl();
    }

So removing that placeholder we won't get any warning/fatal errors, and the links without attributes will just render fine and all will continue working as expected.

I just attached a patch and the interdiff to this comment with my proposal.

Also, I have an idea about how to write the test but first I want to be sure if this is the behaviour to we want.

I will change temporarily the status of the issue to see if the testbot have complaints with my patch.

This fails because the order of the attributes changes :-/

Update-rewrite the tests to me is a big change and now I think I would go with #2

had the same problems after a d6 migration.
patch 2 allows me to run drush ms without error or warning.
patch 22 creates new issues:
Illegal string offset 'href' LinkGenerator.php:159 [warning]
Invalid argument supplied for foreach() Attribute.php:87 [warning]

Drupal 8.0.5 getting a similar error on every page load.

PHP Fatal error: Unsupported operand types in /var/www/d8/core/lib/Drupal/Core/Utility/LinkGenerator.php

Confirmed that Patch #2 solved the issue for me. I had upgraded from D6 to D8. Thanks!

@jnicola it's not best practice to rtbc your own patches, as that misses the idea that stuff is reviewed and tested by someone other than the patch creator. Also we need a test to ensure we don;t introduce this bug again.

+1 on "this patch is necessary and good, ship it" and +1 failing to see why the lack of a test would hold off a minor syntax change that fixes a fatal error effecting numerous users. There is no change in behaviour or interfaces here.

The proper test for this would be to create a situation where $variables['options']['attributes'] is not an array, then the fatal error would be demonstrated. I'm not sure of the utility of such a test though, as even with this fix in place, the test will fail since array_merge will throw a warning.

Tried adding a test like in #34. Quickly copied one of the other tests and started modifying.

EDIT: (to explain the sample)
Here is the line that sets the $variables:
$this->moduleHandler->alter('link', $variables);

The reason that I didn't try to mock the alter() function is that I don't know if it is possible to alter the $variables parameter in a mocked function. That is why I added a new function:
$variables = $this->moduleHandler->returnMyself($variables);

In normal execution, the new function doesn't alter $variables. For the test, it will be mocked and will set $variables['options']['attributes'] to a non array.

Some problems:
1. Had to add a function to moduleHandler so that there was a way of altering $variables. The function returns the object passed, but for tests the function gets mocked and replaces $variables['options']['attributes'] with a non array. There is got to be a better way (to modify the $variables variable).
2. It's not completing tests. For now, I have that part that sets it to a non array commented out, so it should return itself, but it still fails to finish the test.
3. Even if that test were working, it's going to throw a warning.
4. This part might be the wrong way to mock the function: ->will($this->returnCallback(function($object) {

See https://www.drupal.org/core-gates#testing and given that we're exchanging a fatal for a warning it seems all we are doing is treating symptoms rather the cause. Can someone post some steps to reproduce - i.e. how can we cause this problem on a real Drupal site?

@jnicola thanks for to reproduce this hints that we have a problem with the Drupal 6 to 8 migration - we need to work out what's going on there. This is an important problem to fix.

A different though similar fatal error in D6->d8 link migration: https://www.drupal.org/node/2698083

@jnicola Drupal should not produce any warnings or errors after such a migration.

@jnicola

The answer is none BUT we MUST fix the cause of this error not just the symptom, the first thing we need to do here is make a failing test for expose the bug and start working in a patch for fix it.

So looking at the code in CckLink migration process plugin we have:

  /**
   * {@inheritdoc}
   */
  public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
    $attributes = unserialize($value['attributes']);
    // Drupal 6 link attributes might be double serialized.
    if (!is_array($attributes)) {
      $attributes = unserialize($attributes);
    }

    if (!$attributes) {
      $attributes = [];
    }

    // Massage the values into the correct form for the link.
    $route['uri'] = $value['url'];
    $route['options']['attributes'] = $attributes;
    $route['title'] = $value['title'];
    return $route;
  }

It looks like this code is failing us.

I've installed Drupal 6, enabled CCK and link and added a link field to the page content type. I selected the least options I could - i.e. no link title and created some content with a link. I then performed the migration to Drupal 8 and the link came through and the content worked as expected.

@jnicola is there any chance you have the original database values for the link fields that is causing the issue?

Mine is...

select * from content_type_page;
+-----+-----+-------------------+------------------+-----------------------------------------------------------------------+
| vid | nid | field_link_url    | field_link_title | field_link_attributes                                                 |
+-----+-----+-------------------+------------------+-----------------------------------------------------------------------+
|   1 |   1 | http://drupal.org | NULL             | a:3:{s:6:"target";s:7:"default";s:5:"class";s:0:"";s:3:"rel";s:0:"";} |
+-----+-----+-------------------+------------------+-----------------------------------------------------------------------+
1 row in set (0.00 sec)

Tagging as migrate critical.

@alexpott If you set the link url to "test" it'll result in a fatal error post-migration, but that is a different fatal error, the one I reported on #2698083. I verified today on 8.1 HEAD that it still exists.

After reading #2633568: Improve method for migrating link attributes from D6 and seeing that that issue was created around the same time as this one and was committed on March 15th 2016 - maybe the migrate part of this issue is already fixed.

Personally I don't care whether we trigger a recoverable or fatal error here because anything that is populating $variables['options']['attributes'] with something other than an array is breaking the contract of the theme function and is broken. We don't validate the returns of alter hooks in many places.

However given that migrate was broken and making the alter hook return FALSE for the attributes maybe we could change the code to specifically test for that and carry on.

As @alexpott says, we could fix this in core just so we don't have to write some nasty looking update hook or tell everyone to re-migrate.

I think it's most likely that #2633568: Improve method for migrating link attributes from D6 fixed the issue but it's possible that unserialize returned something truthy, but not an array. Maybe we should replace

  if (!$attributes) {
      $attributes = [];
    }

with

  if (!is_array($attributes)) {
      $attributes = [];
    }

Looking at all the code involved with this I think we should consider ensuring that attributes in an array in LinkItem::setValue() because that should fix sites which have already migrated prior to #2633568: Improve method for migrating link attributes from D6 landing and also ensure the link items present a valid options array even if the link generator is swapped out or the options are used somewhere else.

We should avoid the fatal error here, but trigger_error() with a warning or throw a proper exception rather than just erroring out. Drupal 6 has similar fatal errors if the $options argument to l() is NULL instead of an array (which ironically happened a lot on sites updated from Drupal 5).

Since migrate was already fixed, removing tag.

Also moving back to routing.

Patch #2 works for me as well.

Updated patch #2 to apply to Drupal 8.2.0.

So when attributes is not an array this is clearly a bug. Its kind of an implementation detail that array_merge() seems to work with other properties as well.

What about adding a assert() statement to ensure that the bug is caused in testing/development environments, but maybe still use this patch, so runtime systems do stumble upon this particular bug.

There is a way. I have to patch core on every upgrade. Good times!

I'm wondering wether we should have some assertions to ensure that people don't pass in NULL or so in there?

Triaged.
Patch no longer applies to 8.4. Rerolled.

God, reading through this post is depressing. I have let my D6 site sit on D6 forever because D7 migration was awful and just didn't work for me. With Google no longer feeding me traffic because i don't have mobile ready site, because it's sitting on D6 and I can't be bothered to make a poor investment into an unsupported platform, I decided to try upgrading to D8 this past week.

Used Drush to move all 5000 nodes of my content type which includes a Web Link field. Half of my content views just fine, the other half throws the white page, and with error messaging on, reveals:

Fatal error: Unsupported operand types in /var/myserver/path/core/lib/Drupal/Core/Utility/LinkGenerator.php on line 153

Has this issue still not been fixed? I honestly can't be bothered to go through and rebuild this site from scratch. If I have to do that, I'm thinking to just use Node_export on D6 to export everything as CSV files and have a go at Joomla. I was a big user of D6 and had many sites built on it, it just seems things have been down hill from there. Sad ;(

Just run the patch. I'm still using a modified version of git apply link_gen_array_merge-2596937-1.patch that i run on every upgrade. gone from 8.0x->8.3x without issue

Sorry about the frustration everyone.

I'd also agree with #60; the fact that the array_merge() works when HEAD doesn't is a fragile way of fixing it, and the actual bug lies with the caller. (If you're one of the people applying this patch to your site, you might consider debugging where the bad input is coming from, which is also what reviewers are trying to make sure you can do.)

That said, I'd consider a a hotfix and a @todo/followup for a more robust fix and the array_merge(), given the persistence of the issue.

Even if we commit it as a hotfix, though, we do still at minimum need an automated test for what we expect to happen. This isn't inanity -- it's standard practice to ensure this doesn't break again in the future.

P.S. -- I've used Drupal for 12 years; infer what you will.

Take a look at \Drupal\Tests\Core\Utility\LinkGeneratorTest which is a unit test specifically for that class, there are a few examples in there that should point you in the right direction.

Given that $variables['options']['attributes'] is initialised within the generate() method and $variables is local to the method then it can only be someone implementing the alter hook triggered by $this->moduleHandler->alter('link', $variables); that causes the invalid attributes.

Something like this will trigger the same error in a test.

$this->moduleHandler
  ->method('alter')
  ->willReturnCallback(function ($url, &$vars) {
    $vars['options']['attributes'] = '';
  });

I also experienced this problem during the migration from D6 to D8 (yes in 2019 :)). I created an issue/patch to prevent the migration to save an invalid string over here: #3045211

Hello,

We were facing this issue when trying to access the sitemap page, that is being done with the sitemap module.
I've tried to apply the patch from #65, it partially solve the issue, but it was throwing this error after it was applied:
array_merge(): Argument #2 is not an array in Drupal\Core\Utility\LinkGenerator->generate()

So, I've made a change on the patch and will post it here. Hope it should be useful for someone.

Thanks

Sorry, forgot about the space after the 'if'

Thanks, for working on the patch.

+++ b/web/core/lib/Drupal/Core/Utility/LinkGenerator.php
@@ -150,7 +150,9 @@ public function generate($text, Url $url) {
+    if (is_array($variables['options']['attributes'])) {
+      $attributes = array_merge(['href' => ''], $variables['options']['attributes']);

I think adding is_array makes sense here but changing + to array_merge is not quite right. We don't always want to merge stuff e.g. https://3v4l.org/YhXaN. Let's revert it back to + so it doesn't break BC.

Actually the + and the array_merge are almost the same, but there is a difference between them, as pointed out on this comment on the PHP manual to this function: https://www.php.net/manual/en/function.array-merge.php#92602

I've tested here and adding the check is_array and using + it doesn't throw errors anymore.

I've attached the patch if you want to test, for me it worked properly. Hope it does for you too.

I ran into this issue on a site I inherited that a previous developer had migrated from D6 to D8. I don't know why, but it was dormant until recently when it started causing errors and white screening the offending content page. I took a different approach, cleaning up the data from the myself prompt.

I thought the command might come in handy for someone else. You might have to change the filed name to match your own DB.

UPDATE node__field_external_link SET field_external_link_title="", field_external_link_options='a:0:{}' WHERE isnull(field_external_link_title);

I ran into this as well recently and I think this has been addressed in the migrate system for me at least.

The data from D6 was getting migrated with double encoding of the attributes.

I rolled back the migration and reran it and seems to be fixed now!

\Drupal\link\Plugin\migrate\process\attributes

    $attributes = unserialize($value['attributes']);
    // Drupal 6/7 link attributes might be double serialized.
    if (!is_array($attributes)) {
      $attributes = unserialize($attributes);
    }

I agree the current patch though nice, though doesn't fix the problem where the data is incorrect in the database.

I'm going to mark this as fixed/duplicate by #2897254: URLs without http:// are broken after migration from d6 or d7 and #3045211: Prevent link field migration from creating invalid link attributes

Just posting an updated patch to squash the error while I dig through the DB for the permanent fix.