Woocommerce-admin: get_current_page() doesn't work for registerted JS pages

[DanReyLop]

WC_Admin_Page_Controller::get_current_page() can't be used for fragment-based pages.

The problem is that the fragment part of the URL (everything that comes after the #) is never sent to the server. So, the PHP logic here will never be able to correctly identify a "registered" page. It still works for traditional "connected" pages that don't rely on fragment URLs.

Looks like this is related with this @todo too.

At the very least, there should be a caveat in that function's documentation, making it clear that it won't work for registered fragment-based pages, and remove the PHP_URL_FRAGMENT check since it will never work.

Additionally, what is the reason to use fragment-based URLs? As far as I know, using HTML5's History API should allow us to use URLs wc-admin?something=something without refreshing the browser.

cc/ @jeffstieler since you added this on https://github.com/woocommerce/woocommerce-admin/pull/2209

[jeffstieler]

Ah, yeah I completely missed this. It appears to work because the Dashboard page is registered without anything in the fragment. All child pages are then errantly identified as the parent..

[justinshreve]

I did a little exploration on this today (related to https://github.com/woocommerce/woocommerce-admin/pull/2300) and I think we could switch to React Router’s BrowserHistory/ the History API with some tweaks, updates to the URL structure, and a bit of magic on admin_init — so that we can detect routes server side.

https://github.com/woocommerce/woocommerce-admin/compare/try/page-router-update. (Please excuse the rough/messy try branch. There are a number of things that need updated, and we have to hack around the menu handling a bit). You should be able to test by going directly to /wp-admin/admin.php/woocommerce/analytics/products.

I needed to pause on this for now, but it seems like something we could figure out with a bit more work and testing.

[DanReyLop]

Please excuse the rough/messy try branch.

Messiness in a try/ branch is always excused :)

I tried going to /wp-admin/admin.php/woocommerce/analytics/products but I can't get the WC-Admin JS to load. Also, every other menu item on the sidebar is broken because it builds the URL using wp-admin/admin.php/woocommerce/analytics/ as a base. I think using querystring parameters instead of admin.php/woocommerce/* would give us less problems.

I needed to pause on this for now, but it seems like something we could figure out with a bit more work and testing.

Is there anything we can help with? It would be great to be able to use JS-powered "registered" pages from the start.

[justinshreve]

@timmyc This issue is the one we mentioned during the 3.7/wc-admin chat yesterday. I think we should be able to figure something out if we can add this to an upcoming sprint. This is definitely something we'd want to figure out now, because it will be harder to move away later.

[justinshreve]

I have a much cleaner attempt for this up at https://github.com/woocommerce/woocommerce-admin/compare/update/route-handling. It attempts to work with what we've got and put the route in a query string: admin.php?page=wc-admin&path={ route }.

This still feels hacky to me and needs more thought, and/or I think we need to do more of a rewrite of the navigation system. One possible change that we could maybe make is relying on screen IDs instead of URL encoded paths.

Unfortunately, I think this is going to need a bit more attention to get right than I can give it right now with onboarding work. Hopefully this branch can start as a better point for someone to pick-up.

[psealock]

This still feels hacky to me and needs more thought,

Nice work here @justinshreve. That does seem less than ideal. Was the previous attempt a non-starter? The url ends up much nicer, is there a reason it won't work?
https://github.com/woocommerce/woocommerce-admin/compare/try/page-router-update

I can give it a shot this week

[DanReyLop]

Was the previous attempt a non-starter? The url ends up much nicer, is there a reason it won't work?

@psealock Basically, this:

Also, every other menu item on the sidebar is broken because it builds the URL using wp-admin/admin.php/woocommerce/analytics/ as a base.

Mixing fancy PushState-based URLs with plain HTML <a href="page.php"> relative links will backfire in many ways. I think adding an extra querystring parameter is the safest solution, even if the URLs won't be as nice.

I'll take a look at the new branch tomorrow.

[DanReyLop]

@justinshreve It looks much better now :)

Note that this line: https://github.com/woocommerce/woocommerce-admin/blob/172bb3e2074eee4012dcc0c1672fcd61b865aeaa/includes/page-controller/class-wc-admin-page-controller.php#L107
Means that every page in WP-Admin will be considered a wc-admin registered page, so the native WP-Admin sidebar menu will dissapear. I committed on that branch fixing it: https://github.com/woocommerce/woocommerce-admin/commit/7c86b948c20d36c4aa7fe62ffa3b09dcdb9c5e63

Also, my site is installed as blahblahblah.ngrok.io/wordpress/ instead of blahblahblah.ngrok.io, so that was causing problems with navigation. I fixed it here, feel free to trash my solution if you come up with something cleaner: https://github.com/woocommerce/woocommerce-admin/commit/de022e31b9c6756611d6b96d2cf48da37bf3134d

Otherwise, it looks great! Moving from page to page is smooth (it's still JS-powered navigation). Can't wait for this to be a real PR :)

[psealock]

https://github.com/woocommerce/woocommerce-admin/commit/75dd516615d5f89d352f7426fbff76753a8ba253 removes the page parameter from the query param passed down to the app. The extra parameter was getting passed along to api queries.

[psealock]

I created a PR so we can have a more relevant discussion there, https://github.com/woocommerce/woocommerce-admin/pull/2444