Make WordPress Core

Opened 11 months ago

Last modified 7 weeks ago

#63110 reviewing defect (bug)

Site Editor: Update old URLs to new path-based URLs

Reported by: wildworks's profile wildworks Owned by: wildworks's profile wildworks
Milestone: 7.0 Priority: normal
Severity: normal Version: 6.3
Component: Editor Keywords: good-first-bug has-patch needs-test-info
Focuses: Cc:

Description

Related Gutenberg issue: https://github.com/WordPress/gutenberg/issues/69584

Currently, the site editor references the p GET parameter to determine which page to show. However, there are some URLs that use old parameters (postType, path, etc)

It would be good to update these URLs to avoid unnecessary URL redirection.

From what I can see, there are three codes that need to be updated:

Change History (18)

This ticket was mentioned in PR #8514 on WordPress/wordpress-develop by @sainathpoojary.


11 months ago
#1

  • Keywords has-patch added

Trac ticket: #63110

#2 @sainathpoojary
11 months ago

Hey @wildworks,

I have updated the URL for dashboard in the PR. However, the other two instances are related to register_post_type, where the URL is used for _edit_link, which is for internal use so I’m unsure how I can reproduce this to test it.

I converted the URL as per the `_wp_get_site_editor_redirection_url function, but I'm not sure if it's correct. Could you please have a look?

From:

	$navigation_post_edit_link = 'site-editor.php?' . build_query(
		array(
			'postId'   => '%s',
			'postType' => 'wp_navigation',
			'canvas'   => 'edit',
		)
	);
	$template_edit_link = 'site-editor.php?' . build_query(
		array(
			'postType' => '%s',
			'postId'   => '%s',
			'canvas'   => 'edit',
		)
	);

To:

	$navigation_post_edit_link = 'site-editor.php?' . build_query(
		array(
			'p' => '/wp_navigation/%s',
			'canvas' => 'edit',
		)
	);
	$template_edit_link = 'site-editor.php?' . build_query(
		array(
			'p' => '/wp_template/%s',
			'canvas'   => 'edit',
		)
	);

#3 @wildworks
11 months ago

@sainathpoojary

I’m unsure how I can reproduce this to test it.

Template

To access the post list, temporarily run the following hook:

add_filter( 'register_post_type_args', function( $args, $post_type ) {
	if ( $post_type === 'wp_template' ) {
		$args['show_ui'] = true;
	}
	return $args;
}, 10, 2 );

#4 @sainathpoojary
11 months ago

Hey @wildworks,

Thanks for the detailed guidance! I followed the steps to reproduce and test the URLs. Everything seems to be working as expected. I’ve updated the PR accordingly, and it’s now ready for review. Let me know if any further changes are needed.

#5 @wildworks
11 months ago

@sainathpoojary Thanks for submitting a patch!

I checked your PR, and it would be a good idea to encode slashes (/) in the get parameter to %2F.

P.S. Gutenberg's JS utility (addQueryArgs()) automatically encodes URLs so no explicit encoding is needed. On the other hand, I think we would need explicit encoding in PHP.

#6 @wildworks
9 months ago

  • Keywords changes-requested added

#7 @sainathpoojary
9 months ago

Hey @wildworks,

I initially tried using urlencode( '/wp_navigation/%s' ), but that caused issues when passed to sprintf(). The reason is that urlencode() outputs %2F, and when % signs are present in the format string, sprintf() treats them as format specifiers. So having %2F and %s in the same string led to unexpected errors in get_edit_post_link function, since sprintf() tried to interpret %2F as a format placeholder.

To fix this, I used str_replace( '/', '%%2F', '/wp_navigation/%s' ). This escapes the % signs as %% so that sprintf() treats them as literal %, resulting in proper URL-encoded slashes (%2F) in the final output, while keeping %s intact for dynamic replacement.

Let me know if there’s a cleaner or preferred way to handle this.

#8 @wildworks
9 months ago

  • Keywords commit added; changes-requested removed

#9 @westonruter
3 months ago

  • Keywords commit removed
  • Milestone changed from Awaiting Review to 7.0
  • Owner set to wildworks
  • Status changed from new to reviewing

I'm removing commit because the patch probably needs to be re-tested after 7 months.

#10 @jabir20
7 weeks ago

  • Keywords tested added

Tested patch from PR #8514 on WordPress trunk locally.

✅ Navigation editor links:

  • site-editor.php?p=%2Fwp_navigation%2F4&canvas=edit
  • Links open correctly
  • URL uses path-based p= with encoded slashes %2F
  • No redirect observed

✅ Template editor links:

  • site-editor.php?p=%2Fwp_template%2Ftwentytwentyfive%2F%2Findex&canvas=edit
  • Editor opens correctly and saves changes
  • URL uses path-based p= with encoded slashes %2F

Environment:

  • Windows, XAMPP
  • PHP 8.2, MySQL 8
  • Node 24.11 (for asset builds)
  • WordPress trunk

Notes:

  • Template list shows a minor fatal error due to trunk changes (get_delete_post_link()), but this does not affect the tested functionality.
  • Patch works as intended for URL conversion and editor loading.

#11 follow-up: @SirLouen
7 weeks ago

  • Keywords needs-test-info added; tested removed
  • Version set to 6.3

Template list shows a minor fatal error

@jabir20 can you test it using Playground and check if you get that error in the Playground console?

Using this URL:
https://playground.wordpress.net/wordpress.html?pr=8514

Anyway, for testing this, what is the current behaviour for navigation editor links and the pretended behaviour with the patch? Aren't both working rn?

Last edited 7 weeks ago by SirLouen (previous) (diff)

#12 in reply to: ↑ 11 ; follow-up: @jabir20
7 weeks ago

@SirLouen

I tried testing the PR using WordPress Playground at:
https://playground.wordpress.net/wordpress.html?pr=8514

However, Playground currently fails to load the PR and shows:
"The PR 8514 couldn't be previewed due to an unexpected error." https://prnt.sc/M8mh__BsopaK

This seems to be a Playground issue rather than an authentication problem, as no login is required and the error appears immediately.

For clarity:

  • I was able to test the patch successfully on a local trunk environment.
  • Navigation editor links and template editor links behave as expected with the patch.
  • The fatal error I mentioned earlier only appeared locally when exposing wp_template via show_ui, and does not seem related to the URL changes in this patch.

Thanks

Last edited 7 weeks ago by jabir20 (previous) (diff)

#13 in reply to: ↑ 12 ; follow-up: @SirLouen
7 weeks ago

Replying to jabir20:

I tried testing the PR using WordPress Playground at:
https://playground.wordpress.net/wordpress.html?pr=8514

However, Playground currently fails to load the PR and shows:
"The PR 8514 couldn't be previewed due to an unexpected error." https://prnt.sc/M8mh__BsopaK

It seems that Playground is failing right now. Try again later.

#14 in reply to: ↑ 13 @jabir20
7 weeks ago

@SirLouen
I’ve been trying for the last 6–8 hours, and it keeps showing the same error every time.
Thanks

#15 @SirLouen
7 weeks ago

@jabir20 it seems that the problem is that the PR 8514 is too old.
Two options here: Replicate the PR and push it again fresh to test (this is what I generally do), or ask a committer to do a rebase to trigger a build again for the tests.

cc @wildworks @westonruter

#16 @westonruter
7 weeks ago

I think the deeper problem is that the new template activation feature was reverted from being included in 6.9, so the code this is supposed to fix is no longer present in trunk. We'll need to wait for it to be re-added.

#17 @wildworks
7 weeks ago

Since I've merged the latest upstream trunk in the pull request, the playground link should work correctly now.

I think the deeper problem is that the new template activation feature was reverted from being included in 6.9, so the code this is supposed to fix is no longer present in trunk. We'll need to wait for it to be re-added.

This ticket and PR 8514 were submitted before the template activation feature was committed to core. Whether template enablement is in core or not is irrelevant.

#18 @jabir20
7 weeks ago

Thanks everyone @SirLouen , @westonruter , @wildworks for the clarification and for updating the PR.

I re-tested the patch using WordPress Playground now that PR #8514 has been updated:
https://playground.wordpress.net/wordpress.html?pr=8514

✅ Playground testing results:

  • Playground loads correctly now.
  • I did NOT encounter the fatal error that I previously saw in my local trunk setup.
  • This confirms the earlier fatal error was likely environment-specific and not caused by this patch.

✅ Navigation editor links:

  • Links use path-based p= URLs with encoded slashes (%2F).
  • Editor opens directly without redirect.

✅ Template editor links:

  • After enabling show_ui for wp_template, template links open correctly in the Site Editor.
  • URLs use path-based p= routing and save works as expected.

Regarding current vs intended behavior:

  • Navigation and template editors already work today, but rely on legacy query parameters that trigger redirects.
  • This patch improves internal URL consistency by switching to path-based p= URLs, avoiding unnecessary redirects and aligning with Gutenberg routing.

Everything appears to be working as intended with the patch.

Note: See TracTickets for help on using tickets.