Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get menu by id name and slug #30

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fahrradflucht
Copy link
Contributor

Changed /menus/ to /menus/ to take all supported param types of wp_get_nav_menu_object and wp_get_nav_menu_items. This makes the slug filter argument suggested in 6b03a18 obsolete.

Note that this is currently V2 only.

As noted in unfulvio#18 it's helpfull to get the slug of the referenced
object in some cases (e.g. custom routing).

Now every item and possible children have an `object_slug` field.
In case of custom links it just returns the items slug since the
id refers to the item it self.

Note that the v1 implementation isn't tested.

Closes unfulvio#18
In some cases the cost of fetching the menus first and then only
fetching the childs for the menus that are needed by id are higher
then fetching them all at once. It's now possible to specify an
`items` argument to indicate whether to fetch the menu items aswell
or not.

This could be also helpfull in cases like [this](https://wordpress.org/support/topic/get-menu-data-by-name-not-id) to filter on the client, although a `slug` argument to filter the menus array in addition would be need.

Note that this change is currently V2 only.
Changed /menus/<id> to /menus/<menus> to take all supported param
types of [wp_get_nav_menu_object](https://developer.wordpress.org/reference/functions/wp_get_nav_menu_object/) and [wp_get_nav_menu_items](https://developer.wordpress.org/reference/functions/wp_get_nav_menu_object/).
This makes the slug filter argument suggested in 6b03a18 obsolete.

Note that this is currently V2 only.
@fahrradflucht
Copy link
Contributor Author

Note that this PR is already based on #28 and #29.

@fahrradflucht
Copy link
Contributor Author

This change shouldn't cause users apps to break since it still allows hitting routes like /menus/:id and get the expected resource.

Despite that fact there could be the hypothetical case in which a user queries the /menus/:id route with some generated stuff and expects to get an empty array in case it isn't an ID which wouldn't happen anymore if that stuff is a menu name or menu slug. I don't know why someone would do that though.

All in all I won't count that as breaking.

westonruter added a commit to xwp/wp-api-menus that referenced this pull request Jul 15, 2016
@fahrradflucht
Copy link
Contributor Author

fahrradflucht commented Nov 21, 2016

@mckernanin I would merge this but I think it would be great if I could get a second per of eyes before that.

Let me know if you need any additional information or context to make this easier for you.

@mckernanin
Copy link

I will check it out

@hetenho
Copy link

hetenho commented Mar 10, 2017

Any progress here? Would love to see this PR merged. I´ve tested this behaviour locally with id/name/slug and it works great 👍

@jedrzejchalubek
Copy link

jedrzejchalubek commented Aug 2, 2017

For everyone who's waiting for this and needs additional fields on returned items. We can use a filter for now.

function menus_api_filtering($item) {
    $item['object_slug'] = get_post($item['object_id'])->post_name;

    return $item;
}
add_filter( 'rest_menus_format_menu_item', 'menus_api_filtering' );

@drewcovi
Copy link

@unfulvio Any reason this is held up?

@unfulvio
Copy link
Owner

I'm sorry I'm not actively following development of this plugin anymore since a couple of years and I'm no longer actively checking on PRs or tickets.

Unfortunately I'm not doing much front end work with the WP REST API anymore and I have no time to code review or user test the changes, let alone manage the project direction.

I've opened up to other developers who'd like to maintain the project or fork it and I'd be very happy to add collaborators who are willing to commit to it. Some should have already push access.

If anyone needs me to merge a PR I will do it. But it's not going to be tested or reviewed by me. Same goes for plugin updates on the WordPress.org repository.

Thanks for understanding.

@drewcovi
Copy link

Certainly understandable. This pull request takes care of the issue. I’ve promoted contributors to my own repos when dealing with work / life / github balance.

If I were you, I’d simply promote @fahrradflucht if he’s game to pick this one up a bit.

Would also help if he can also commit to the WordPress plugin repo.

@unfulvio
Copy link
Owner

He is already a collaborator with push access. Please confirm you're able to merge this. I can also add on WP SVN for plugin update purposes.

@fahrradflucht
Copy link
Contributor Author

I am able to merge this but I don't do any wordpress (API) development either. 😕 I hoped that @mckernanin would review and merge this.

@unfulvio
Copy link
Owner

Thanks for chiming in. Would love to find new maintainers.

After all this plugin is relatively popular, I'm pretty sure there must be someone that actively uses it for stuff like clients work and would like to have it maintained. That was the case for me until I switched companies and moved to different projects.

Apart from time, one reason I'm unfit to continue sustaining it is that I no longer have connection with clients work and real use cases that would show me how to improve the plugin thanks to users feedback.

I'd probably rewrite some parts to make it more robust. Check whatever changes went into WP API, and figure out why they didn't add native support for menus (the main motivation why I wrote this extension back then was that they didn't support menus and my clients needed it -- I thought in fact the project would have been much more short lived).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants