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.)
Comment | File | Size | Author |
---|---|---|---|
#42 | php5.4.png | 61.82 KB | droplet |
#39 | menu-link-save-1338282-39.patch | 1.39 KB | pillarsdotnet |
#30 | menu-link-save-1338282-30-d7.patch | 970 bytes | JamesOakley |
#26 | menu-link-save-1338282-26-d7.patch | 970 bytes | JamesOakley |
#15 | menu-link-save-1338282-15.patch | 1.39 KB | pillarsdotnet |
Comments
Comment #1
rfayI suspect this needs a more detailed issue summary.
Comment #2
ELC CreditAttribution: ELC commentedCan't you jsut use the error suppression if you *know* it's bullshit? (having not actually looked at the code, but only the patch)
(assuming array_intersect_assoc is the thing throwing errors)
Comment #3
chx CreditAttribution: chx commentedIt's not 100% bullshit there is an epsilon chance of someone actually changing only options and then trying to save.
Comment #4
zymsys CreditAttribution: zymsys commentedI 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:
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.
Comment #6
zymsys CreditAttribution: zymsys commentedWell 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.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis would be just fine (and seems to be the intend of the code):
array_intersect_key($item, $existing_item)) != $existing_item
Comment #8
zymsys CreditAttribution: zymsys commentedI 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.
Comment #9
zymsys CreditAttribution: zymsys commentedI'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.
Comment #10
chx CreditAttribution: chx commentedWe 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?
Comment #11
zymsys CreditAttribution: zymsys commentedI 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.
Comment #12
chx CreditAttribution: chx commentedThe 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.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commented#7 is still what we want :)
Comment #14
chx CreditAttribution: chx commentedComment #15
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch as suggested in #7.
Comment #15.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedproper summary
Comment #16
pillarsdotnet CreditAttribution: pillarsdotnet commentedTitle change reflecting intent of latest patch.
EDIT: Tests blocked by #61456-86Yay! HEAD is unb0rken!
Comment #17
pillarsdotnet CreditAttribution: pillarsdotnet commentedSame 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.
Comment #18
droplet CreditAttribution: droplet commented#7 comparison is good enough in this case
Comment #20
droplet CreditAttribution: droplet commented#15: menu-link-save-1338282-15.patch queued for re-testing.
Comment #21
droplet CreditAttribution: droplet commentedfailed patch is D7
Comment #22
xjmI gather that since this is specific to 5.4, we can't make a test that will fail on the bot?
Comment #23
webchickHm. 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.
Comment #24
droplet CreditAttribution: droplet commentedIt's tests but testbot isn't PHP 5.4
Comment #25
webchickClosed #1477646: "Array to string conversion in menu_link_save" error with PHP 5.4 as a duplicate.
Comment #26
JamesOakleyThis 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.
Comment #28
JamesOakleyor 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!
Comment #29
webchickTestbot 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!
Comment #30
JamesOakleyThanks, 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.
Comment #31
droplet CreditAttribution: droplet commentedD8 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.
Comment #32
PedroMiguel CreditAttribution: PedroMiguel commentedTested the d7 patch on several existing sites and one new instalation and all works well. Thanks.
Comment #33
xjm@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?
Comment #34
JamesOakleyWebchick 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.
Comment #35
klonosI 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?
Comment #36
JamesOakley@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.
Comment #37
droplet CreditAttribution: droplet commented@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
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedMarked #1483986: When Installed Drupal 7.12 with PHP 5.4.0 Support got the error ": Illegal string offset 'field' in DatabaseCondition->__clone() as duplicate
Comment #39
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled
Comment #40
JamesOakleyI'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.
Comment #41
pillarsdotnet CreditAttribution: pillarsdotnet commented@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.
Comment #42
droplet CreditAttribution: droplet commentedokay. re: @xjm:
before:
after:
652 passes, 0 fails, 0 exceptions, and 158 debug messages
Comment #43
JamesOakley@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?
Comment #44
pillarsdotnet CreditAttribution: pillarsdotnet commented@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.Comment #45
JamesOakleyThanks - 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 ;)
Comment #45.0
JamesOakleyUpdated to reflect the latest patch.
Comment #46
pillarsdotnet CreditAttribution: pillarsdotnet commentedConfirmed with chx on irc that his original report was specific to PHP 5.4. Edited issue summary accordingly.
Comment #46.0
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-worded to make it clear that this problem is php 5.4 specific.
Comment #47
pillarsdotnet CreditAttribution: pillarsdotnet commentedRemoving tag.
Comment #48
klonos#44:
...related #1171958: Allow files to be assigned to branch(es)/version(s) and thus tested against it
Comment #49
archit.lohokare CreditAttribution: archit.lohokare commentedphp54_can_die_in_a_fire.patch queued for re-testing.
Comment #50
webchickThanks for looking into that, folks!
Committed and pushed to 8.x and 7.x. Thanks!
Comment #51
klonosThanx Angie! One less patch to "babysit" ;)
Comment #52
valthebaldCosmetic 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.
Comment #53
JamesOakleyI wouldn't have thought you'd need either paranthesis. I certainly agree that the parantheses around the call to array_intersect_key are redundant.
Comment #54
xjmYeah, that looks like a typo. You could open a followup novice issue for someone to fix that to make the order more explicit. :)
Comment #55
valthebaldThe question is, remove parenthesis or leave them to describe intended order?
Comment #56
xjm@valthebald -- We can discuss that in the new issue, but I'd say keep the parens and and make the change in #52.
Comment #57
valthebaldNew issue is #1516030: Change parenthesis in menu_link_save() to express intended order of operations
Comment #58.0
(not verified) CreditAttribution: commentedNo test possible.