Recoverable fatal error: Argument 3 passed to l() must be an array, string given with references_dialog [#2315905]

There's some sort of weird interaction with bootstrap and references_dialog theming of action links. With core bartik no issues. After switching to bootstrap any node with a references_dialog widget can't be edited (maintenance error page displays instead) and results in the following watchdog entry:

Recoverable fatal error: Argument 3 passed to l() must be an array, string given, called in C:\inetpub\xxx\includes\theme.inc on line 1737 and defined in l() (line 2426 of C:\inetpub\xxx\includes\common.inc).

I've traced it down to the info added to the links array processed by theme_links. The bartik array looks like this:

while the bootstrap array looks like this:

It barfs on the non-null icon_position item that has a value of "before" when looping through the array with foreach ($links as $key => $link) in theme_links. I guess it safely ignores null items but I'm confused as to why there's a mixture of indexed and associative array items being processed with a foreach $index => $value loop in core to begin with. Seems like that's asking for trouble like this.

I have no clue what the proper way to fix it is yet, but a work around is to override core's theme_links in the bootstrap subtheme's template.php file and adding the following line right after the foreach ($links as $key => $link):

    foreach ($links as $key => $link) {
      if (!is_integer($key)) continue; //temp fix
      $class = array($key);

      // Add first, last and active classes to the list of links to help out themers.
      if ($i == 1) {
        $class[] = 'first';
      }
      if ($i == $num_links) {
        $class[] = 'last';
      }
      if (isset($link['href']) && ($link['href'] == $_GET['q'] || ($link['href'] == '<front>' && drupal_is_front_page())) && (empty($link['language']) || $link['language']->language == $language_url->language)) {
        $class[] = 'active';
      }
      $output .= '<li' . drupal_attributes(array('class' => $class)) . '>';
....

Anyway, I just wanted to get this posted in case anyone else stumbles across it-- took quite a bit debugging to figure out. I'll try to figure out a proper fix when I get a change.

In the meantime, suggestions would be welcome ;-)

Comment	File	Size	Author
#42	references_dialog_fix_theme_links-2315905-42.patch	1.78 KB	Steven Merrill
#20	references_dialog_beta1-fix_theme_links-2315905-20.patch	1.78 KB	achton
#10	references_dialog_fix_theme_links-2315905-10.patch	1.88 KB	natemow
#5	references_dialog_fix_theme_links-2315905-5.patch.patch	1.78 KB	WorldFallz
#3	references_dialog_fix_theme_links-2315905-3.patch	1.23 KB	WorldFallz
	bartik-links-array.png	6.55 KB	WorldFallz
	bootstrap-links-array.png	10.62 KB	WorldFallz

Comments

Comment #1

markhalliwell

English

commented 5 August 2014 at 18:43

Project:	Bootstrap	» References dialog
Version:	7.x-3.x-dev	» 7.x-1.x-dev

This isn't a Bootstrap base-theme issue. We're not overriding theme_links() in any way, we're just adding the .list-inline class when appropriate: http://cgit.drupalcode.org/bootstrap/tree/templates/system/links.vars.php.

From http://cgit.drupalcode.org/references_dialog/tree/references_dialog.modu...

function theme_references_dialog_links($links) {
  return theme('links', array('links' => $links, 'attributes' => array('class' => 'references-dialog-links')));
}

This module is not using the theme system API properly. It is arbitrarily passing the argument $links (which should actually be $variables btw) directly as the links parameter to the theme_links() hook. This will, of course, blow up. $variables can include various items (including some added by base-themes).

This is what theme_links() expects as it's links parameter:

links: An associative array of links to be themed. The key for each link
is used as its CSS class. Each link should be itself an array, with the
following elements:

title: The link text.

href: The link URL. If omitted, the 'title' is shown as a plain text
item in the links list.

html: (optional) Whether or not 'title' is HTML. If set, the title
will not be passed through check_plain().

attributes: (optional) Attributes for the anchor, or for the <span>
tag used in its place if no 'href' is supplied. If element 'class' is
included, it must be an array of one or more class names.

If the 'href' element is supplied, the entire link array is passed to
l() as its $options parameter.

Comment #2

WorldFallz commented 5 August 2014 at 19:24

Thanks for the excellent description Mark! Now that I know where/what the bug is, I can submit a patch for it.

Comment #3

WorldFallz commented 5 August 2014 at 20:28

Status	File	Size
new	references_dialog_fix_theme_links-2315905-3.patch	1.23 KB

This should do it.

Comment #4

markhalliwell

English

commented 5 August 2014 at 20:39

Status:

Active

» Needs work

+++ b/references_dialog.module
@@ -326,7 +326,7 @@ function references_dialog_process_widget(&$element) {
-    $element['#suffix'] = '<div class="dialog-links ' . $element['#id'] . '">' . theme('references_dialog_links', $dialog_links) . '</div>';
+    $element['#suffix'] = '<div class="dialog-links ' . $element['#id'] . '">' . theme('references_dialog_links', array('links' => $dialog_links)) . '</div>';

This is correct. However there is one other place this needs to happen as well:
http://cgit.drupalcode.org/references_dialog/tree/references_dialog.modu...

+++ b/references_dialog.module
@@ -696,8 +696,8 @@ function references_dialog_get_admin_path($entity_type, $op, $bundle = NULL, $en
+  return theme('links', array('links' => $variables['links'], 'attributes' => array('class' => 'references-dialog-links')));

FYI, classes should always be inside an array:

array('class' => array('references-dialog-links'))

Comment #5

WorldFallz commented 7 August 2014 at 15:16

Status:

Needs work

» Needs review

Status	File	Size
new	references_dialog_fix_theme_links-2315905-5.patch.patch	1.78 KB

Thanks for the review, I can't believe I missed those. Updated patch attached.

Comment #6

markhalliwell

English

commented 7 August 2014 at 17:52

Status:

Needs review

» Reviewed & tested by the community

Looks good to me, simple and straightforward.

Comment #7

socialnicheguru commented 23 September 2014 at 00:16

could this be related: https://www.drupal.org/node/1978118
(long shot but thought I'd mention it since this issue arises from 'some sort of weird interaction'.)

Comment #8

vacilando commented 7 November 2014 at 23:55

Bumped into this problem today right after updating from Drupal 7.32 to 7.33. Applied to patch and the problem went away!

Could someone please commit this to dev?

Comment #9

designerbrent commented 12 November 2014 at 19:55

This works perfectly to fix the problem. Would love to see this patch committed.

Comment #10

natemow commented 14 November 2014 at 15:17

Status	File	Size
new	references_dialog_fix_theme_links-2315905-10.patch	1.88 KB

Re-rolled patch for beta1 release if anyone needs that instead.

Comment #11

14 November 2014 at 15:23

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 10: references_dialog_fix_theme_links-2315905-10.patch, failed testing.

Comment #12

acrosman

English

South Carolina, US

commented 14 November 2014 at 21:07

Status:

Needs work

» Reviewed & tested by the community

Looks like the patch in #10 is build from site root instead of the module root.

The patch in #5 applies just fine to the Beta1 release; I don't think it needed any re-rolling.

Setting back to reviewed by community in reference to patch from 5.

Comment #13

markhalliwell

English

commented 14 November 2014 at 21:10

4 files were hidden/shown/deleted

Status	File	Size
hidden	bootstrap-links-array.png	10.62 KB
hidden	bartik-links-array.png	6.55 KB
hidden	references_dialog_fix_theme_links-2315905-3.patch	1.23 KB
hidden	references_dialog_fix_theme_links-2315905-10.patch	1.88 KB

Comment #14

anybody

German

Porta Westfalica

commented 17 November 2014 at 12:17

The patch works fine and I can confirm it solves the issue.

I'll be happy when the new stable version is released. :)

Comment #15

gmclelland commented 19 November 2014 at 20:17

The patch in #5 fixes the "Recoverable fatal error" I was getting when I tried to edit nodes after upgrading from 7.32 to 7.33.

Comment #16

morganl commented 20 November 2014 at 02:27

Priority:

Normal

» Critical

Comment #17

morganl commented 20 November 2014 at 02:28

This bug prevents sites from upgrading to Drupal 7.34, a security issue release. Please push a patch to the stable revision ASAP. Thanks!

Comment #18

rp7 commented 20 November 2014 at 14:24

Patch works. Get this in ASAP please.

Comment #19

jesss commented 20 November 2014 at 17:12

The patch in #5 works for me. Please commit!

Comment #20

achton

Denmark

commented 20 November 2014 at 19:12

Status	File	Size
new	references_dialog_beta1-fix_theme_links-2315905-20.patch	1.78 KB

The patch in #5 did not apply cleanly for me on beta1 (contrary to some comments above). And the reroll in #10 is not correctly formatted. So here's a new patch for beta1, which applies cleanly (I'm using an archaic version of Drush Make here, YMMV).

Comment #21

20 November 2014 at 19:19

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 20: references_dialog_beta1-fix_theme_links-2315905-20.patch, failed testing.

Comment #22

20 November 2014 at 19:40

Status:

Needs work

» Needs review

achton queued 20: references_dialog_beta1-fix_theme_links-2315905-20.patch for re-testing.

Comment #23

20 November 2014 at 19:46

Status:

Needs review

» Needs work

The last submitted patch, 20: references_dialog_beta1-fix_theme_links-2315905-20.patch, failed testing.

Comment #24

markhalliwell

English

commented 20 November 2014 at 19:48

Status:

Needs work

» Reviewed & tested by the community

1 file was hidden/shown/deleted

Status	File	Size
hidden	references_dialog_beta1-fix_theme_links-2315905-20.patch	1.78 KB

Patches should always be against the latest HEAD (7.x-x.x branch, -dev), which is why the patch in #20 is failing tests. The patch in #5 should get committed.

Comment #25

achton

Denmark

commented 20 November 2014 at 20:50

Hmm, ok - I was trying to decipher the output from the bots and expected it to fail at "Detect a non-applicable patch" like with the patch in #10. Sorry 'bout the detour!

Comment #26

kristen pol

she/her

English

Santa Cruz, CA, USA

commented 20 November 2014 at 21:25

RTBC++ Thanks, WorldFallz!!!!!!!

Comment #27

WorldFallz commented 20 November 2014 at 21:31

And thanks for Mark for mentoring me through this fix! Now to get it committed... I'm willing to be a comaintainer if necessary....

Comment #28

wroxbox commented 20 November 2014 at 22:28

Fixes the errors. Makes Node/add edit forms again usable. Thanks @WorldFallz

Comment #29

damienmckenna

TN, USA

commented 21 November 2014 at 16:44

Issue tags:

+core incompatibility

I couldn't find another tag for indicating a compatibility problem with specific core releases, so this'll have to do.

Comment #30

morganl commented 25 November 2014 at 20:44

Note: This affects 7.x-1.0-beta1 as well, not just 7.x-1.x-dev.

Comment #31

markhalliwell

English

commented 25 November 2014 at 20:59

-_- it affects everything, this issue/patch hasn't been committed to anything yet. 7.x-1.x-dev is just the repo's HEAD (aka: latest code) which is what patches should be applied to: https://www.drupal.org/patch/create

I have contacted both maintainers asking them to take action here.

Comment #32

morganl commented 25 November 2014 at 21:39

Thank you, Mark.

Comment #33

26 November 2014 at 09:09

blackandcode queued 5: references_dialog_fix_theme_links-2315905-5.patch.patch for re-testing.

Comment #34

fmitchell commented 8 December 2014 at 06:15

If you're still getting this issue after applying the patch to either beta1 or 7.x-dev, it is also because you are probably running <= PHP 5.3 per https://www.drupal.org/node/2379771.

Upgrading to > PHP 5.3 (PHP 5.5 in my case) fixed the issue. If that is not an option, try applying the core patch referenced in the link or wait until 7.35 is released assuming that patch is included.

Comment #35

markhalliwell

English

commented 8 December 2014 at 16:14

Closed #2379771: Adding 'theme_hook_original' triggers an unexpected PHP5.3 string conversion bug in theme_links as it's not a core issue.

Comment #36

nicxvan commented 8 December 2014 at 19:09

I applied this patch, I'm not seeing any results though.

Comment #37

rogical commented 15 December 2014 at 13:52

@WorldFallz, I've added you as a maintainer, thanks!

Comment #38

15 December 2014 at 15:55

WorldFallz committed ef71913 on 7.x-1.x

Issue #2315905 by WorldFallz, achton, natemow, Mark Carver: Recoverable...

Comment #39

WorldFallz commented 15 December 2014 at 15:56

Thanks for the vote of confidence rogical! I've committed this patch to dev. I plan to go through the issue queue and commit any other patches that are essentially RTBC'd before tagging a new official release.