The documentation for the menu_unserialize function describes a $data parameter of array('view', 1) then describes a return value that would be appropriate to a $data parameter of array('node_load',1). Also, the api filters triggered by the 'unserialize(' appear to be corrupting part of the descriptive text. Trivial patch to fix both problems attached.

CommentFileSizeAuthor
#10 408882-10.patch2.95 KBjhodgdon
#8 408882-8.patch2.96 KBjhodgdon
#5 408882.patch2.97 KBjhodgdon
menu.inc_.patch683 bytespillarsdotnet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brianV’s picture

brianV CreditAttribution: brianV commented
Status: Active » Reviewed & tested by the community

Trivial patch. Documentation now matches function. RTBC

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Version: 6.x-dev » 7.x-dev

Get committed to Drupal 7 and backport after that please.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

Hm. While I agree this is fixing a bug in the documentation, I wonder if we could instead re-phrase the docs here so they... make any sort of sense. ;)

"if $data is a serialized array('view', 1) and $map is array('node', '12345') then 'view' will not be changed because it is not an integer, but 1 will as it is an integer."

Um. What? :)

This whole function documentation could stand to be re-written so that it answers the questions:

1. What the heck does this do?
2. Why the heck would I ever want to use it?
3. What's a thorough example of how core is using it? (I kind of get what it's alluding to here, but not quite)

pillarsdotnet, any chance you're up for this, since you obviously understand the function well enough to spot errors in its documentation?

pillarsdotnet’s picture

pillarsdotnet CreditAttribution: pillarsdotnet commented

@Gabor -- Sorry; don't know if D7 even has the same function. Still trying to wrap my head around D6, months after the upgrade.

@webchick -- Yeah, the explanation threw me, too. I had to hand-simulate the code to figure it out, and thee weeks later I neither exactly remember nor wish to repeat the experience. IIRC, the function is used internally to convert menu_router database records to PHP class data which is then cached. There's no reason to use it outside that context.

Finding and fixing problems like this one is a side-effect of my usual debugging strategy:

  1. Insert code block:
    if (!assert("something-assumed-to-be-true")) { dump-backtrace-to-log; dump-relevant-variables-to-log; temporary-workaround; }
  2. Forget about it until the next thing goes wrong.
  3. Read through logs, which are full of debugging code from the last problem.
  4. Backtrack through the traces and fix whatever it was that violated the assertion.
  5. Find the exact code that is producing the current problem.
  6. Trace the chain of assumptions upstream as far as possible.
  7. Go back to step 1.

The next time I get to step 3, I promise to try and document my findings a bit better. Unfortunately, that won't help the D7 guys, but I'm not smart enough to brave D7 on a production website. Not yet.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
408882.patch2.97 KB

How about this version?

jhodgdon requested that failed test be re-tested.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

#5: 408882.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
FileSize
408882-8.patch2.96 KB

The patch above no longer applied. Here's a reroll. I also fixed a couple of other issues, like wrapping the lines closer to 80 characters, and fixing wording in a couple of spots.

This still needs review. THe last patch was about a year old. Hmph.

Status: Needs review » Needs work

The last submitted patch, 408882-8.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review
FileSize
408882-10.patch2.95 KB

doh!

Status: Needs review » Needs work

The last submitted patch, 408882-10.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Needs review

#10: 408882-10.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Not sure what "failed to drop checkout database" means, but I don't think it was due to this patch?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

#10: 408882-10.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

#10: 408882-10.patch queued for re-testing.

mr.baileys’s picture

mr.baileys
Primary language Dutch
Location 🇧🇪 (Ghent)
CreditAttribution: mr.baileys commented
Status: Needs review » Reviewed & tested by the community

Reviewed the patch and found it to be a great improvement over the current documentation for menu_unserialize.

The new documentation does a good job of answering the questions raised by webchick in #3 (although this is a function that has little use outside of the Drupal's menu internals, so the question "Why the heck would I ever use it?" is probably best answered with "You shouldn't" :), and contrary to the existing docs the updated text actually makes sense after carefully reading it. As an added bonus it adds the missing one-line summary.

We could expand even further and give an actual example:

menu_unserialize(
  array('node_load', 1),  // $data
  array('node', 123),      // $map
);
// Returns array('node_load', 123);

... but for a function that's used only by Drupal core itself, I think the last paragraph is sufficient.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

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