Nice!

Would it be better to add a uri field in addition to the existing url, instead of changing the expectation (and breaking behavior) of either url (a fully qualified url) and path (the relative path on the server)?

This is how User objects do it:

@justlevine I added a new uri field rather than modifying the pre-existing path as suggested.

Here's how it looks now

Hey @josephfusco , I tweaked your code a bit to follow the patterns used for other uri fields. Can you confirm it still works on multisite? (It should be fine, but multisite support is still a bit spotty ).

It seems that MenuItems aren't really tested, so I'm gonna take a minute to backfill those before marking this ready.

Coverage increased (+0.3%) to 79.781% when pulling 2e0604d on josephfusco:fix-menu-path-for-subdirectory-ms into ccaf4e6 on wp-graphql:develop.

Hey @justlevine I greatly appreciate your help with this!

After giving this more thought, I'm going to close this out add to fix to the faustwp WordPress plugin. It feels like the more appropriate place to put this seeing that faustwp is the one that is replacing the subdirectory.

@josephfusco I believe this should stay open. While its true that Faust is doing a bad replace on the url, there are a lot of use cases for a relative uri, and it makes the object behave more uniformly with other Nodes that can have an associated link.

If you prefer, I can create a separate PR - lemme know!

@justlevine Works for me! Feel free to keep it on this PR :)

@jasonbahl not sure why the schema linter is failing 🤔

@justlevine looks probably related with this latest release of the schema linter: https://github.com/cjoudrey/graphql-schema-linter/releases/tag/v3.0.0

looks like we'll need to explicitly install graphql in the workflow now

I just fixed the Schema linter in #2368

@jasonbahl And it's passing again 🚀. Ready for your review :)

I think this looks good, but the blocker for me is that our test environments aren't setup to test in multisite at the moment.

I'll open another issue to track that. Once we have the ability to run tests in multisite, we should be able to get this properly tested and merged.

Ah issue for multisite tests here: #2275

Ok, so I got #2375 merged, so now we have tests running in multisite!

I'll work up a test for this so we can get this merged.

I'll work up a test for this so we can get this merged.

Only thing that should be missing is an assertEquals() for $actual['data']['menuItem']['path'];

I left some notes about the uri field / UniformResourceIdentifiable Interface.

I think I'm ok with having the uri field as a non-breaking change here, but I think we should remove the UniformResourceIdentifiable interface, as this Type is not actually identifiable by a uri. The nodes that MenuItem links to are identifiable by the uri, but not the MenuItem itself.

I pushed up a commit to remove UniformResourceIdentifiable from the MenuItem type

Code Climate has analyzed commit 2e0604d and detected 1 issue on this pull request.

Here's the issue category breakdown:

View more on Code Climate.

Pushed a follow-up commit adding the uri field back to the MenuItem Type since it was removed when removing the UniformResourceIdentifiable Interface from the Type.

Now, we can query the uri field of a MenuItem, but MenuItem will not be shown as a possibleType in nodeByUri queries.

I think we still might need to have a specific test where we do something like the following:

• create a new blog in the multisite
• switch to blog
• create a menu with some menu items
• assign the menu to a location (so it's public)
• query the menu
• assert that the blog directory is not in the uri

🤔