Make WordPress Core

Opened 13 years ago

Last modified 20 months ago

#18289 reviewing task (blessed)

Direct link to plugin installation should have admin chrome

Reported by: nacin's profile nacin Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: Cc:

Description

We should be able to provide a direct link to the page to install a plugin, based on the plugin's slug. This does it: wp-admin/plugin-install.php?tab=plugin-information&plugin=log-deprecated-notices. However, there's no admin chrome, no real indication which site you're on, and no name of the plugin.

If we're not loading that page inside an iframe request, it needs the admin around it, as well as a heading. Probably new styling too.

This would serve as a replacement for Jaquith's bookmarklet, which broke in 3.2 (frame busting), as well as allow us to integrate a link on extend/plugins for plugin installation. Related, #16923, which is now closed.

Attachments (8)

18289_1.patch (17.7 KB) - added by csixty4 13 years ago.
Patch to provide requested functionality
18289.diff (6.2 KB) - added by andrewryno 13 years ago.
18289.2.diff (8.2 KB) - added by andrewryno 13 years ago.
18289.3.diff (8.1 KB) - added by andrewryno 13 years ago.
18289.4.diff (28.3 KB) - added by andrewryno 13 years ago.
18289.5.diff (11.8 KB) - added by dd32 12 years ago.
18289.6.diff (11.8 KB) - added by dd32 12 years ago.
typo fix
18289.7.diff (3.6 KB) - added by desrosj 20 months ago.

Download all attachments as: .zip

Change History (29)

@csixty4
13 years ago

Patch to provide requested functionality

#1 @csixty4
13 years ago

  • Owner set to nacin
  • Status changed from new to reviewing

The attached patch adds this functionality. Just add "&wrapped" to the end of the query string.

Couple concerns I have:

  • install_plugins_pre_plugin-information is already an action. To get a return value, I had to make it a filter. Didn't want to break existing plugins, so now it's an action AND a filter, with one called instead of the other if iframed. Kinda redundant. Is it worth adding a separate filter for this, or maybe deprecating the action?
  • Lots of HTML mixed with code in wp-admin/includes/plugin-install.php. Any chance a templating system could be used here?

#2 @SergeyBiryukov
13 years ago

  • Keywords has-patch added

#3 @dd32
13 years ago

Surely theres a better way than returning a huge chunk of HTML.

The first thought that came to my mind, was to just have iframe_header() / iframe_footer() include the normal header/footer, however we'd run into some globals issues there.. (Theres a ticket elsewhere for that I believe)

We know the list of actions which can end up in a iframe, and we should be able to detect if it's a iframe'd request (just throwing an extra param into the url for example).

I'll see if i can manage a patch following my thoughts..

@andrewryno
13 years ago

#4 @andrewryno
13 years ago

Patch is to remove thickbox on the install plugin page (first pass). I was working on this when nacin let me know about this ticket. I'm going to try to solve the no-chrome issue along with this since I'm messing with the plugin information page anyway.

#5 @goto10
13 years ago

  • Cc dromsey@… added

#6 @andrewryno
13 years ago

Updated ticket to show chrome based on no-chrome URL parameter that the AJAX call uses. Then just some logic on where to add_action and if it should exit or not. Played with a couple other ideas of handling it and this was the easiest.

Also, the styling is horribly botched. Made me wonder if we should just redesign this page slightly since it will now show within the admin chrome. Make it fit in a little better? Then could use the same styling within the slide down as well.

@andrewryno
13 years ago

@andrewryno
13 years ago

#7 @andrewryno
13 years ago

Also, just thought: since there will now be an in-site plugin information page, we should just link there for no-js, not wp.org (which I changed it to). Updated patch with that.

#8 @andrewryno
13 years ago

Refreshed patch.

  • Improved styling.
  • Make it work properly on Multisite
  • Some other miscellaneous improvements

@andrewryno
13 years ago

#9 @andrewryno
13 years ago

I'm also repeating a lot of code in that. I think that a lot should be broken up into separate functions but I wanted to get an initial patch up before I started messing with it too much.

#10 @helenyhou
13 years ago

  • Cc helen.y.hou@… added

#11 @sabreuse
13 years ago

  • Cc sabreuse@… added

@dd32
12 years ago

#12 @dd32
12 years ago

  • Keywords needs-testing added

attachment 18289.5.diff added

  • Midnight patch
  • Thickbox requests get a iframe'd version, direct access gets a full admin chrome
  • Revealed a bug in the JS, looks like the Thickbox wasn't getting it's custom title from the plugins page thanks to the list tables using a different table class
  • Had to switch from body_id (which only the iframe handler supports it seems) to a generic body class which is better anyway
  • Doesn't do any Ajax magic like the other patches here
  • Aims for minimal disruption of existing code
  • Seems to work, but needs some extra testing and eyes

Lets not go into doing any UI+UX overhauls/changes here, All install/update functionality needs a facelift, and that needs some thought put into it, I don't think we should go through attempting any of that here

#13 @dd32
12 years ago

  • Avert your eyes from the magic that is a static variable and a function hooked to 2 actions in install_plugin_information_direct()...

@dd32
12 years ago

typo fix

#14 @nacin
12 years ago

$href = add_query_arg('plugin', $_REQUESTplugin?, $href) doesn't look secure especially without escaping in the next line.

#15 @dd32
12 years ago

The patch as-is misses the Plugins widget on the dashboard, and the Importers screen as well.

As for $_REQUEST['plugin'] that should be sanitized with sanitize_key() probably, or just urlencode it instead. (add_query_arg() not encoding/sanitizing things strikes me again..)

I'm not too fussy on this, If someone definitely wants this in 3.3, it'd be appreciated for a patch refresh/update with the above items in mind.

#16 @ocean90
12 years ago

Still planned for 3.3?

Last edited 12 years ago by ocean90 (previous) (diff)

#17 @nacin
12 years ago

  • Milestone changed from 3.3 to Future Release

#18 @iseulde
9 years ago

#29935 was marked as a duplicate.

#19 @chriscct7
8 years ago

  • Keywords needs-refresh added

This ticket was mentioned in Slack in #meta by gibrown. View the logs.


3 years ago

@desrosj
20 months ago

#21 @desrosj
20 months ago

  • Keywords needs-refresh removed
  • Milestone set to Future Release
  • Owner nacin deleted

Wipes the dust off

Found this one going through a list of tickets missing a milestone. Mulling it over, I think this is still something that would be a nice to have, but definitely a low priority item.

I did some testing with the latest version of WordPress (currently 6.0.1), and wp-admin/plugin-install.php?tab=plugin-information&plugin=query-monitor still works to pull up the plugin details in the same way as when this was opened (no admin context).

The easiest way to accomplish this without introducing a new UI would be to introduce a new "no menu tab" for displaying one or more specific plugins.

18289.7.diff is a proof of concept patch that allows a comma separated list of plugins to be passed and displayed in the normal plugin view.

An example URL would be: wp-admin/plugin-install.php?plugin=query-monitor,classic-editor,disable-big-image-threshold&tab=plugin-list.

Some pros of this approach:

  • Uses the same UI.
  • Allows only specific plugins to be listed (search does not even guarantee the intended plugin is listed first).
  • User can click more details on any of them, and also has an install button to click.
  • Allows plugin and theme authors to link to a specific listing of compatible plugins or add-ons.

Some cons:

  • The plugin_information action returns way more information than needed, and is missing some information for display (plugin icons, for example, as seen in the POC patch).
  • There's no specific API action to just receive a specific set of plugins.
  • Results in one API request per plugin specified. Would be wasteful if many (5, 10, 15+ plugins) are listed.

This could also be adjusted to only support one plugin. It felt reasonable to allow multiple, though.

Last edited 20 months ago by desrosj (previous) (diff)
Note: See TracTickets for help on using tickets.