Make WordPress Core

Opened 11 years ago

Closed 9 years ago

Last modified 7 years ago

#23367 closed enhancement (fixed)

Remove message parameters from admin URl's in the browser address bar

Reported by: mark-k's profile mark-k Owned by: ocean90's profile ocean90
Milestone: 4.2 Priority: low
Severity: minor Version:
Component: Administration Keywords: good-first-bug has-patch
Focuses: administration Cc:

Description

Scenario: When a post is being published successfully "message=6" is appended to the URL. If I refresh the page while in that state I will get the "post published" message again, although nothing was done.

There is probably nothing functionally wrong with the way things work now but it can be esthetically more pleasant if that parameter was removed which in turn will eliminate the message problem.

This can be done on browsers that support the history API of HTML5. The following piece of code can be used after displaying the message or maybe it can be generalized to work in the admin footer.

<script type="text/javascript" charset="utf-8">
  url = the canonical URL for the address
  if (typeof history.replaceState === 'function') { // check html5 functionality support
    data = {dummy:true};
    history.replaceState(data,'',url);
  }
</script>

Attachments (12)

src.diff (1.4 KB) - added by VarunAgw 10 years ago.
Code required for fix
common.js.patch (1.6 KB) - added by VarunAgw 10 years ago.
23367-refresh.diff (2.1 KB) - added by morganestes 9 years ago.
Refreshed patch for wp-admin/js/common.js
23367.tests.diff (1.9 KB) - added by morganestes 9 years ago.
First run at unit tests for refresh patch.
23367.refresh1.diff (8.2 KB) - added by morganestes 9 years ago.
Unified patch of working version.
23367.refresh2.diff (8.1 KB) - added by morganestes 9 years ago.
Unified patch, without my test filter example. :/
23367.refresh3.diff (6.0 KB) - added by morganestes 9 years ago.
Refresh with formatting changes suggested by @drewapicture
23367.4.diff (1.6 KB) - added by morganestes 9 years ago.
Canonical URL filtered with PHP
23367.5.diff (1.6 KB) - added by morganestes 9 years ago.
Put it all into a single function since it's limited use.
23367.6.diff (1.7 KB) - added by morganestes 9 years ago.
Changes based on johnbillion's recommendations
23367.7.diff (1.7 KB) - added by morganestes 9 years ago.
Adds conditional check for disabled filter/empty array of removable_query_args.
23367.8.diff (1.8 KB) - added by ocean90 9 years ago.

Download all attachments as: .zip

Change History (57)

#1 @johnbillion
10 years ago

  • Component changed from General to Administration
  • Focuses administration added
  • Keywords needs-patch good-first-bug added
  • Priority changed from normal to low

This seems like a sane idea. We should introduce a JavaScript version of remove_query_arg() to remove the unwanted query vars from the current URL. Here's a starting point for the default list of vars to remove.

Any takers?

Last edited 10 years ago by johnbillion (previous) (diff)

#2 @VarunAgw
10 years ago

  • Keywords needs-patch removed

I have created a patch for this. I am unsure where to add this. I think maybe wp-admin/js/common.js but I am not sure. I have attached it.

@VarunAgw
10 years ago

Code required for fix

#3 @nacin
10 years ago

  • Owner set to VarunAgw
  • Status changed from new to assigned

Hi VarunAgw: common.js seems like a good place, yep!

@VarunAgw
10 years ago

#4 @VarunAgw
10 years ago

  • Keywords has-patch added

This ticket was mentioned in IRC in #wordpress-dev by VarunAgw. View the logs.


10 years ago

#6 @ilonaF
9 years ago

  • Keywords needs-refresh added

The patch file does not apply anymore since the code around has change significantly since. Needs updating.

#7 @wonderboymusic
9 years ago

  • Keywords needs-patch added; has-patch needs-refresh removed
  • Milestone changed from Awaiting Review to Future Release

The list of query args could be output into JSON on the page that the script could read. That way: PHP and JS could read the same list of vars and future-proof themselves.

#8 @SergeyBiryukov
9 years ago

  • Summary changed from Remove message parameters fron admin URl's in the browser address bar to Remove message parameters from admin URl's in the browser address bar

@morganestes
9 years ago

Refreshed patch for wp-admin/js/common.js

@morganestes
9 years ago

First run at unit tests for refresh patch.

#9 @morganestes
9 years ago

  • Keywords has-patch added; needs-patch removed

I refreshed the common.js.patch to bring it more inline with core standards and write a unit test for it.

This is my first unit test for core, so I'd love some feedback on it. I tested just a single instance of what the URL query stripper would encounter (params in the list removed, not in the list stay).

#10 @DrewAPicture
9 years ago

  • Owner changed from VarunAgw to morganestes

#11 follow-up: @wonderboymusic
9 years ago

  • Keywords needs-refresh added

@morganestes: thanks! see: https://core.trac.wordpress.org/ticket/23367#comment:7

common.js should be localized using wp_localize_script() to read the query params to remove, instead of listing them in the file. This future-proofs the code when new query params are added.

location doesn't need to be wrapped in jQuery, just read window.location.href

I would use this as an opportunity to introduce some query string JS functions to read all of the params and parse them into a map - something that could possibly be added into wp-util.js. That way you could do:

params = get_query_params(); <----- not a good name
_.each( localizedParams, function (key) {
    delete params[ key ];
} );

Or use some Underscore function to get the difference between the 2 based on keys.

#12 in reply to: ↑ 11 @morganestes
9 years ago

Replying to wonderboymusic:

@morganestes: thanks! see: https://core.trac.wordpress.org/ticket/23367#comment:7

Sorry, read that last night and immediately forgot it when I started working on the code. I'll work on implementing it.

common.js should be localized using wp_localize_script() to read the query params to remove, instead of listing them in the file. This future-proofs the code when new query params are added.

What's the preferred approach for the JS object: create a new one or attach it to an existing one? If existing, which one is best for general use?

location doesn't need to be wrapped in jQuery, just read window.location.href

Yep, makes sense. I'll make the switch.

I would use this as an opportunity to introduce some query string JS functions to read all of the params and parse them into a map - something that could possibly be added into wp-util.js. That way you could do:

params = get_query_params(); <----- not a good name
_.each( localizedParams, function (key) {
    delete params[ key ];
} );

Or use some Underscore function to get the difference between the 2 based on keys.

I'll see what I can do. I'm imagining this being the utility function that's used to generate the JSON output mentioned above.

@morganestes
9 years ago

Unified patch of working version.

#13 @morganestes
9 years ago

  • Keywords needs-unit-tests added; needs-refresh removed

Here's the summary of what I came up with:

  • Added utility functions to the wp.url namespace object to handle URL string manipulations and placed it in wp-utils.js.
  • Localized the script to pass an array of parameters to remove from the query string.
  • Created a filter for the array of parameters so they can be modified outside the main code.
  • Added a global Object wpUrlParams that contains a map of the original parameters and their values prior to being filtered.
  • Note that the actual URL filter relies on document.pushState, which isn't supported in IE 9 or lower without a polyfill. Those browsers will get access to wpUrlParams but not the URL changes, which is current behavior.

Here's what filtering the blacklist would look like:

add_filter( 'removable_url_params', function( $removable_url_params ) {
	$my_params = array( 'mdub' );
	return array_merge( $my_params, $removable_url_params );
});
Last edited 9 years ago by morganestes (previous) (diff)

@morganestes
9 years ago

Unified patch, without my test filter example. :/

@morganestes
9 years ago

Refresh with formatting changes suggested by @drewapicture

This ticket was mentioned in Slack in #design by helen. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by morganestes. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by morganestes. View the logs.


9 years ago

#18 follow-up: @helen
9 years ago

I take it from the patch that Underscore.js would now be loaded everywhere in the admin? I also find the name switching in wp_localize_script( 'common', 'removableQueryArgs', $removable_url_params ) to be a little odd, as a nitpick :)

#19 follow-up: @iseulde
9 years ago

Maybe this is a stupid question, but why do we do a redirect with query args instead of looking at POST?

This ticket was mentioned in Slack in #core by morganestes. View the logs.


9 years ago

#21 in reply to: ↑ 18 @morganestes
9 years ago

Replying to helen:

I take it from the patch that Underscore.js would now be loaded everywhere in the admin?

I didn't realize it wasn't already. If not, should we be or should I be using jQuery since it already is?

I also find the name switching in wp_localize_script( 'common', 'removableQueryArgs', $removable_url_params ) to be a little odd, as a nitpick :)

Nitpicking is good here. I changed names a few times and forgot to settle on a single one. I'm thinking removableQueryArgs and $removable_query_args, but am open to suggestions if you have any particular leanings.

#22 in reply to: ↑ 19 @morganestes
9 years ago

Replying to iseulde:

Maybe this is a stupid question, but why do we do a redirect with query args instead of looking at POST?

I'm not sure I follow. We're not doing a redirect, just a location replacement if the browser supports it so we don't get hit with the same message on a refresh.

#23 @iseulde
9 years ago

I'm not sure I follow. We're not doing a redirect, just a location replacement if the browser supports it so we don't get hit with the same message on a refresh.

Server side. :)

This ticket was mentioned in Slack in #core-editor by azaozz. View the logs.


9 years ago

#25 @markjaquith
9 years ago

Summarizing some feedback I gave @morganestes in Slack:

  1. This approach sets a global wp-admin list of URL parameters that can't be used for anything other than transient message passing. If any plugin is using one of these for non-transient information (like selecting a plugin admin menu's sub-tab), a user refresh will result in that query parameter being gone, with unexpected results. It’s not the end of the world, but something to keep in mind.
  1. We shouldn't be doing all this work in JavaScript. We have PHP functions for removing query args. We should just make a function that, in the <head> section, outputs two things:
<link id="wp-admin-canonical" rel="canonical" href="<?php echo wp_admin_canonical_url(); ?>" />

and

<script>if ( typeof history.replaceState === 'function' ) { history.replaceState( null, null, document.getElementById('wp-admin-canonical').href ); }</script>

And then wp_admin_canonical_url() would be something like this:

function wp_admin_canonical_url() {
    $parameters_to_remove = array(
    'message',
    'updated',
    'settings-updated',
    'saved',
    'activated',
    'activate',
    'deactivate',
    'locked',
    'skipped',
    'deleted',
    'trashed',
    'untrashed',
    );
    $parameters_to_remove = apply_filters( 'removable_url_params', $parameters_to_remove );
    return remove_query_arg( $parameters_to_remove );
}

Reasons:

  1. Less code.
  2. Not duplicating something in JS that we already have in PHP.
  3. rel=canonical is a standard practice, and I think this is a good use of it.
  4. No Underscore.js dependency on every admin screen. Indeed, this has no dependencies.

This ticket was mentioned in Slack in #core by mark. View the logs.


9 years ago

#27 @DrewAPicture
9 years ago

  • Keywords needs-patch added; has-patch removed

#28 @DrewAPicture
9 years ago

  • Milestone changed from Future Release to 4.2

@morganestes
9 years ago

Canonical URL filtered with PHP

#29 @morganestes
9 years ago

  • Keywords has-patch added; needs-patch removed

New patch with changes suggested by @markjaquith is attached with a couple of things to note:

  • Since canonical links must not be relative, force an absolute URL with get_site_url() inside wp_admin_filtered_url(). Otherwise, remove_query_arg() returns a relative URL.
  • Use jQuery.isFunction() to check for history.replaceState support as recommended by JS coding standards.

@morganestes
9 years ago

Put it all into a single function since it's limited use.

#30 @morganestes
9 years ago

@markjaquith: A couple of changes introduced in Patch 5:

  • roll the filter and the HTML/JS output into a single function since it's single-use, rather than the filter function standing alone and only being used to populate the canonical link.
  • build the current URL manually, then pass that to remove_query_arg() (same process used elsewhere in core), rather than prepending get_site_url() to build the absolute canonical URL.
  • after some discussion with @DrewAPicture, @azaozz, and @iseulde, switch to if ( window.history.replaceState ) {} for feature detection instead of jQuery.isFunction or doing a typeof check.

This ticket was mentioned in Slack in #core by morganestes. View the logs.


9 years ago

#32 @johnbillion
9 years ago

  • Keywords needs-patch added; needs-unit-tests has-patch removed

23367.5.diff looks solid to me. I agree that this can go into one function because of its narrow use.

The filter name should be changed to removable_query_args so it's more inline with the add|remove_query_arg() function names.

We can also add update, enabled, and disabled to the list of removable query args (ref).

@morganestes
9 years ago

Changes based on johnbillion's recommendations

#33 @morganestes
9 years ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#35 @ocean90
9 years ago

  • Keywords needs-refresh added
  • Owner changed from morganestes to ocean90
  • Status changed from assigned to reviewing

Needs a refresh per todays bug scrub.

@morganestes
9 years ago

Adds conditional check for disabled filter/empty array of removable_query_args.

#36 @morganestes
9 years ago

  • Keywords needs-refresh removed

#37 @morganestes
9 years ago

@ocean90 I'm going to skip the equality check for current and filtered URLs since there's some issues with $_SERVER['REQUEST_URI'] being filtered elsewhere in core, so equality isn't a guarantee like we want it to be.

I've refreshed the patch with @helen's conditional check for an empty $removable_query_args, so the script tag isn't printed if the filter is effectively disabled.

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

@ocean90
9 years ago

#39 @ocean90
9 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 31736:

Administration: Remove single-use URL parameters and create canonical link based on new URL.

The default removable query args are 'message', 'settings-updated', 'saved', 'update', 'updated','activated', 'activate', 'deactivate', 'locked', 'deleted', 'trashed', 'untrashed', 'enabled', 'disabled', and 'skipped'.

props morganestes.
fixes #23367.

#40 @dd32
9 years ago

In 31882:

When altering the admin URL to reflect the canonical location, keep the existing hash (if present) in the URL.
Fixes #31758. See #23367

#41 @helen
9 years ago

In 31973:

Admin notices: Make (most) core notices dismissible.

These no longer return upon refreshing the page when JS is on and working, so users should be able to dismiss them. This is particularly important on the post edit screen when DFW is triggered, but pretty much all notices can be dismissed if needed. A post on Make/Core will follow with information on how this can be leveraged in plugins.

props valendesigns, afercia, paulwilde, adamsilverstein, helen.
fixes #31233. see #23367.

#42 @helen
9 years ago

Oops, meant to mention in the commit above that I added spammed and unspammed for comments.

#43 @westonruter
7 years ago

In 40313:

Customize: Prevent links to customize.php from being generated which have query vars from wp_removable_query_args() present.

Props dlh.
See #23367, #32692.
Fixes #31850.

#44 @swissspidy
7 years ago

In 40331:

Customize: Prevent links to customize.php from being generated which have query vars from wp_removable_query_args() present.

Props dlh.
See #23367, #32692.
Fixes #31850.

Merges [40313] to the 4.7 branch.

#45 @dtbaker
7 years ago

Just a note: this replaceState call on every page has a nice side effect. It prevents the browser re-submitting POST data on refresh/back button, which is really quite cool (especially dealing with poorly written plugins that do not redirect after submitting a form).

It took me so long to figure out why I could refresh after a POST and the browser would do a GET without prompting to resubmit post data. This only happened with JS enabled.

Details/examples here: https://stackoverflow.com/questions/45656405/browser-prevent-post-data-resubmit-on-refresh-using-only-javascript https://dtbaker.net/files/prevent-post-resubmit.php

Shweet!

Note: See TracTickets for help on using tickets.