Problem/Motivation

In PHP 5.4, If you change only the options of a menu link and try to save it back it doesn't save. Also, the code checking for this throws a notice.

Proposed resolution

Refactor the checking code to avoid the notice and fix the problem (same code does it). Or decide we dont care about the bug and slap a @ in front of the array_intersect_assoc. The first is attached.
Replace the array_intersect_assoc() call with array_intersect_key() to properly handle the multi-dimensional components of a menu link array.

Remaining tasks

A test is needed.

If the testbot runs PHP 5.4, existing tests are sufficient. If not, no test is possible.

User interface changes

None.

API changes

Previously changing options of a menu link followed by a menu_link_save didnt save. Now it does.
(Not sure if this is still true with the latest patch, but at least the PHP 5.4 notice is gone.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rfay’s picture

Issue tags: +Needs backport to D7

I suspect this needs a more detailed issue summary.

ELC’s picture

Can't you jsut use the error suppression if you *know* it's bullshit? (having not actually looked at the code, but only the patch)

if (!$existing_item || (@array_intersect_assoc($item, $existing_item)) != $existing_item) {

(assuming array_intersect_assoc is the thing throwing errors)

chx’s picture

It's not 100% bullshit there is an epsilon chance of someone actually changing only options and then trying to save.

zymsys’s picture

Status: Needs work » Needs review
FileSize
2.22 KB

I would propose a different solution to this problem. I was looking at it and came across another issue resulting from the same change in PHP 5.4:

http://drupal.org/node/1342216

I propose adding a drupal version of array_intersect_assoc() which performs a deep comparison, and which can be used to solve this whole class of problem:

/**
 * Computes the intersection of arrays with additional index check
 * 
 * Unlike the native PHP array_intersect_assoc function, this function
 * performs a deep comparison of nested arrays and objects by using
 * serialize() instead of casting values to strings.
 * 
 * @param $array1
 *   The array with master values to check.
 * @param $array2
 *   An array to compare values against.
 * @param $array
 *   A variable list of arrays to compare.
 *   
 * @return
 *   Returns an associative array containing all the values in array1 that 
 *   are present in all of the arguments.
 */
function drupal_array_intersect_assoc($array1, $array2) {
  $haystack = array_slice(func_get_args(), 1);
  $matchtarget = func_num_args() - 1;

  $intersection = array();
  foreach ($array1 as $key=>$value) {
    $matchcount = 0;
    foreach ($haystack as $array) {
      if (array_key_exists($key, $array)) {
        if (serialize($value) == serialize($array[$key])) {
          $matchcount += 1;
        }
      }
    }
    if ($matchcount == $matchtarget) {
      $intersection[$key] = $value;
    }
  }
  return $intersection;
}

Edit: I've added the missing return statement. D'oh.

I have tested this against the latest 8.x from git and it passes tests. I have also tested that the example in the PHP manual continues to work as expected, and I have tested against the example given in the related PHP bug report here:

https://bugs.php.net/bug.php?id=60198

My function gives the expected output from the bug report.

I'm new to Drupal, so I wasn't sure how to submit this patch since it isn't a new issue but an alternative to an already submitted patch. Let me know what you think. I've attached what would have been my patch.

Status: Needs review » Needs work

The last submitted patch, core-menu-save-1338282-4.patch, failed testing.

zymsys’s picture

Well I sure feel stooopid. Obviously that function, and the one in the patch isn't the one I tested successfully - it doesn't return $intersection.

Now there's something wrong with my test environment and all tests are failing... Give me a little time and I'll sort it out and resubmit.

Damien Tournoud’s picture

This would be just fine (and seems to be the intend of the code):

array_intersect_key($item, $existing_item)) != $existing_item

zymsys’s picture

I guess this does boil down to the intent of the code. The comment above the offending line indicates that it wants to check values, which array_intersect_key() does not do. I'm not clear on the use case which causes menu options to not save (I'm not very familiar with Drupal) - I was just trying to fix the problem described by the notice.

Adding
return $intersection;
to the bottom of my function should fix it, but I want to make sure tests pass before I try to resubmit my patch. I'm still having trouble there because in the most recent version from git and PHP 5.4RC1 the tests time out, or if I turn off timeouts run the first test until I give up (about an hour). I'm running tests of an unmodified current snapshot with PHP 5.3.5 and so far have 5 tests failing at 25% complete. I'm going to test my updated patch against that and see if it at least doesn't break anything that wasn't already broken.

There are other parts of Drupal including Views (see http://drupal.org/node/1342216) also affected by this change in PHP 5.4 and I'm not sure if array_intersect_key() will be appropriate for all cases.

zymsys’s picture

I've given up on getting all the tests to run off the latest git version... This one at least allows the menu tests to pass for me with the updated patch.

The only difference between this and the last one is that I've added the required return to the function.

chx’s picture

Status: Needs review » Needs work

We don't need a deep check here, the code IMO is slow (in an already veyr slow process) and finally it'd make array(1,2) and array(2,1) differ. I disagree with the whole approach, in short. What's wrong with the original patch posted?

zymsys’s picture

I think there is a philosophical difference in our approaches. The original patch is IMHO very specialized and addresses a specific problem instead of a class of problems. I don't see how performance optimization is appropriate in this case, which appears to be limited to installation and updating menus. Your example of the difference between array(1,2) and array(2,1) makes my point - different sort orders are different and should be detected as a change when array values are considered - generally speaking. A general solution may also help other issues such as http://drupal.org/node/1342216.

That said, you are infinitely more familiar with Drupal than I am, and I defer to your experience with regard to this issue.

chx’s picture

The problem is that whether array(1,2) and array(2,1) is to be considered equal or not is very specific to the problem. But, more often than not, we do not care about array order. This is what makes necessary to use various trickery because PHP doesnt provide an "all key-value pairs of the array are equal" operator.

Solving this in a generic way as a D8 feature makes sense but not as a backport-to-D7 bug fix IMO.

Damien Tournoud’s picture

#7 is still what we want :)

chx’s picture

Assigned: chx » Unassigned
pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
1.39 KB

Patch as suggested in #7.

pillarsdotnet’s picture

Issue summary: View changes

proper summary

pillarsdotnet’s picture

Title: menu links don't save if only options change » Fix php 5.4 notice in menu_link_save()

Title change reflecting intent of latest patch.

EDIT: Tests blocked by #61456-86
Yay! HEAD is unb0rken!

pillarsdotnet’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests, -PHP 5.4

Same notice happens in PHP 5.2.17.

Retracted -- I may have been confused about which version I was running, and d8 does not support PHP 5.2 anyhow.

droplet’s picture

Title: Fix php 5.4 notice in menu_link_save() » Fix php notice in menu_link_save()
Status: Needs review » Reviewed & tested by the community
Issue tags: +PHP 5.4

#7 comparison is good enough in this case

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs backport to D7, -PHP 5.4

The last submitted patch, menu-link-save-1338282-15-d7.patch, failed testing.

droplet’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7, +PHP 5.4

#15: menu-link-save-1338282-15.patch queued for re-testing.

droplet’s picture

Status: Needs review » Reviewed & tested by the community

failed patch is D7

xjm’s picture

I gather that since this is specific to 5.4, we can't make a test that will fail on the bot?

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

Hm. Not sure. The OP seems to imply that this caused a problem regardless of PHP 5.4. Could someone take a look? If tests are indeed impractical, feel free to mark back to RTBC.

droplet’s picture

It's tests but testbot isn't PHP 5.4

webchick’s picture

JamesOakley’s picture

This D7 patch fixed it for me - just did a straight-forward backport to D7 from the D8 patch in #15 there.

Will this fail the testbot? Don't know, but the problem with the D7 patch in #15 was (I think) the D8-style Core folder in the file identifiers.

Status: Needs review » Needs work

The last submitted patch, menu-link-save-1338282-26-d7.patch, failed testing.

JamesOakley’s picture

or not... One feature of drupal.org I'm yet to grasp is exactly how the testbot works. At least I have a patch that works for me!

webchick’s picture

Status: Needs work » Needs review

Testbot uses the version selector from the issue in order to determine which branch it should test against. So in this case, since you uploaded a D7 patch against an issue marked for "8.x-dev", the test failed because the patch doesn't apply to 8.x. In these circumstances, it's best practice to upload the patch with "do-not-test.patch" at the end, because then testbot will ignore it.

http://drupal.org/node/1058140 is our documentation page about how testbot works. If you'd like to edit it to make it more clear, fill your boots!

JamesOakley’s picture

Version: 8.x-dev » 7.x-dev
FileSize
970 bytes

Thanks, webchick, for your patience. The further I get along the path into contributing to Drupal, the more I start to feel I ought to understand these things. I'm grateful for those who are happy to point in the right direction. (I see that documentation page said, until very recently, that putting -D7 on the end of a patch name allows the testbot to work with the correct core version, but that it now documents the "-do-not-test" suffix you mention).

Reading the comments above, it seems that #15 is an acceptable solution for D8 (seeing as the issue was tagged as needing backport), and passed the testbot. So the remaining issue is finding a D7 patch - let's try again. The attached just now applied for me with a freshly cloned copy of Drupal 7.x, so if it fails it shouldn't be for failing to apply.

droplet’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Reviewed & tested by the community

D8 first.

I marked #15 as RTBC because we already have tests on this function. testbots isn't use PHP 5.4, so that we never hit errors. feel free back to NW if needs one more confirms

@JamesOakley,
#1092232: Bot needs to handle patches named for all core versions -D[678], sadly it only changed the description at the end.

PedroMiguel’s picture

Tested the d7 patch on several existing sites and one new instalation and all works well. Thanks.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

@droplet: Please see #23. There is some indication this is not a PHP 5.4-only issue, so we need confirmation of that before we can go in without additional tests. The question is not whether or not we have some existing test coverage.

Is there a test that fails on 5.4 without this patch, and passes with it?

JamesOakley’s picture

Webchick marked #1477646: "Array to string conversion in menu_link_save" error with PHP 5.4 as a duplicate of this issue. That is probably correct, as the suggested solution to this issue solved the presenting problem there.

However, note that the issue there was very simple - Drupal 7 will not install in a PHP 5.4 environment without throwing in excess of 400 errors. That is something that a test bot should be able to establish; a human tester (like me) can certainly reproduce that.

The only reason why #1477646: "Array to string conversion in menu_link_save" error with PHP 5.4 may not be a duplicate is that the issue here is far more involved. Solving this bigger problem takes time, and there is a need, IMO, to get Drupal 7 or 8 installing with PHP 5.4 without generating errors.

klonos’s picture

I got this multiple times with the latest D7 dev on Windows with php 5.4:

Notice: Array to string conversion in menu_link_save() (line 3147 of C:\www\includes\menu.inc).

The patch in #30 (manually applied) solved the issue. Thanx. Can we get this in?

JamesOakley’s picture

@klonos - I agree.

The problem we've got is that this issue queue concerns a different problem (to do with menu settings not being saved properly in certain circumstances). It just happens that one of the proposed solutions for this also fixes the PHP 5.4 incompatibility that you and I both stumbled over. Rightly, the patch at #30 isn't going to be committed as part of this issue until it has been determined that the patch solves the full issue being discussed.

While we're waiting for that, which seems to be not straight forward, we're left with a major PHP problem for Drupal 7 core. I think I'll re-open #1477646: "Array to string conversion in menu_link_save" error with PHP 5.4 on the grounds that, whilst it relates to this issue, the specific issue there has a higher priority than this one, so needs attention in its own right. Someone may disagree and close it again as a duplicate, but my view is that the issues are related but separate.

droplet’s picture

@xjm,
please see #31. that's answered your question.

simpletest catches "notice". any simple call to menu_link_save(), it's an error.

anyway, test PHP5.4 is much easier than ever:
1. download
2. run build-in server
http://php.net/manual/en/features.commandline.webserver.php

pillarsdotnet’s picture

Re-rolled

JamesOakley’s picture

I'm trying to review this patch, so I've downloaded Drupal 8 (-dev branch), installed, created one page, added a menu link to the main menu, gone to Structure > Menus > Main Menu > List Links and then clicked on "Edit" next to the link entry for the node I just created. I'm running PHP 5.3.10 for this test.

I tried toggling to the "enabled" option. I tried toggling the "show as expanded" option. I tried changing the weight and the parent item. In each case, clicking "Save", it preserved the setting fine.

I'd gladly test this, but I need a more detailed brief as to how to reproduce the exact bug. The top of this issue queue says simply "If you change only the options of a menu link and try to save it back it doesn't save." I thought that's what I'd checked.

Please can someone tell me how to reproduce the problem with the current dev release of D8. I'll then happily test the patch and confirm, or not, that the patch solves the problem.

pillarsdotnet’s picture

@JamesOakley -- The error only shows up in PHP 5.4 or later. Earlier versions of PHP have a bug which enables the code to work without error.

droplet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
61.82 KB

okay. re: @xjm:

before:

php5.4.png

after:
652 passes, 0 fails, 0 exceptions, and 158 debug messages

JamesOakley’s picture

@pillarsdotnet: I used 5.3.10 because the issue title was a while back to reflect the fact that this is not just a PHP 5.4 issue.

Having posted my failure to reproduce the issue, I then switched over to PHP 5.4.0. I got the error messages about array to string conversion, but still could not reproduce the menu settings failing to persist.

I see that droplet has since tested this, and it's been moved to RTBC.

What happens next. Can I re-roll that patch for Drupal 7, or do we wait for the Drupal 8 version to be committed first?

pillarsdotnet’s picture

@JamesOakley: Regarding the original report, I dunno. Ask chx.

The D8 patch must be committed first. If you re-roll for D7 before then, please name your patch menu-link-save-1338282-d7-do-not-test.patch so that the issue doesn't unnecessarily get put into a "needs work" status.

JamesOakley’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs tests, +PHP 5.4

Thanks - I'll wait for the D8 commit in that case, just in case 7.x changes in the meantime. No point making work for myself ;)

JamesOakley’s picture

Issue summary: View changes

Updated to reflect the latest patch.

pillarsdotnet’s picture

Confirmed with chx on irc that his original report was specific to PHP 5.4. Edited issue summary accordingly.

pillarsdotnet’s picture

Issue summary: View changes

Re-worded to make it clear that this problem is php 5.4 specific.

pillarsdotnet’s picture

Issue tags: -Needs tests

Removing tag.

klonos’s picture

#44:

The D8 patch must be committed first. If you re-roll for D7 before then, please name your patch menu-link-save-1338282-d7-do-not-test.patch so that the issue doesn't unnecessarily get put into a "needs work" status.

...related #1171958: Allow files to be assigned to branch(es)/version(s) and thus tested against it

archit.lohokare’s picture

php54_can_die_in_a_fire.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for looking into that, folks!

Committed and pushed to 8.x and 7.x. Thanks!

klonos’s picture

Thanx Angie! One less patch to "babysit" ;)

valthebald’s picture

Cosmetic question:
Should it be
if (!$existing_item || (array_intersect_key($item, $existing_item)) != $existing_item)
or
if (!$existing_item || (array_intersect_key($item, $existing_item) != $existing_item))
?
First case just surrounds array_intersect_key() call with parenthesis, which makes no sense. Second case explicitly tells intended order of operations.

JamesOakley’s picture

I wouldn't have thought you'd need either paranthesis. I certainly agree that the parantheses around the call to array_intersect_key are redundant.

xjm’s picture

Yeah, that looks like a typo. You could open a followup novice issue for someone to fix that to make the order more explicit. :)

valthebald’s picture

The question is, remove parenthesis or leave them to describe intended order?

xjm’s picture

@valthebald -- We can discuss that in the new issue, but I'd say keep the parens and and make the change in #52.

valthebald’s picture

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

Anonymous’s picture

Issue summary: View changes

No test possible.