Log1x / navi

A developer-friendly alternative to the WordPress NavWalker.
https://github.com/Log1x/navi
MIT License
311 stars 29 forks source link

array keys and item id values no longer match in generated array #66

Closed daverobertson closed 5 months ago

daverobertson commented 1 year ago

Hi,

In the readme's example output, the array's keys and "id" values match, and correspond to the ID of the menu item created in WP. In the example, it starts at 5, 6, etc.

Since (I think) 2.0.4, the keys and id values don't match. For example, the generated array now seems to increments from 1, 2, etc.. which doesn't correspond to the IDs of the menu items created in WP.

This broke a few implementations for us, since we'd set array items based on the menu item ID (e.g. if a particular menu item should be active in certain contexts).

A quick workaround restores the previous behavior:

// generate the array like usual:
$nav = (new Navi())->build('primary')->toArray();

// workaround: set array keys to match menu item ids:
foreach ($nav as $id => $item) {
    $nav[$item->id] = $item;
    unset($nav[$id]);
}

Thanks!

Log1x commented 1 year ago

I did change that as it was an oversight on my part to not have them properly ordered to begin with.

Previously, ordering items in the backend had the potential to be in the wrong order when built by Navi so it is now using the order value as the key since you can already get the ID from the item its self.

I'm sorry I didn't make that more clear as a potential breaking change in the release notes. 😦

daverobertson commented 1 year ago

Got it!

We've never had an issue with Navi mis-ordering things before, but I understand.

Since this is by design, we'll hang on to our workaround since we do need to modify things based on the menu item's ID. This way the user can move their menu items around. Perhaps it can help others...

Cheers,

Log1x commented 1 year ago

I might've been over-thinking it when I did the change and there was perhaps no issue with having the ID's as it was before since they will always be unique menu ID's. 🤔

I'm not opposed to reverting the behavior – but is there any reason your implementation cant just use $value->id instead of using the key?

daverobertson commented 1 year ago

Yes; it just makes it easier to update an item when we can reference a unique ID / key. I'm not sure how to do that via $value->id other than looping through the array to evaluate each menu item?

Here's a simplified example of our use case. Sometimes we want to highlight a menu item based on a certain criteria (custom post type, template, special pages, etc):

// $nav is already set by this point.

if (some_condition()) {
    $nav[MENU_ID_ABOUT]->activeAncestor = true;
}

That key could be a constant defined somewhere (as seen here), or a variable set via a custom UI. If the menu item ID is 42, we can reference it directly and easily manipulate the $nav array.

Thanks for considering! Let me know if I can supply any more info,