Opened 22 months ago

Last modified 19 months ago

#18289 reviewing task (blessed)

Direct link to plugin installation should have admin chrome

Reported by: nacin Owned by: nacin
Priority: normal Milestone: Future Release
Component: Upgrade/Install Version:
Severity: normal Keywords: has-patch needs-testing
Cc: dromsey@…, helen.y.hou@…, sabreuse@…

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 (7)

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

Download all attachments as: .zip

Change History (24)

Patch to provide requested functionality

  • 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?
  • Keywords has-patch added

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..

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.

  • Cc dromsey@… added

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.

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.

Refreshed patch.

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

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.

  • Cc helen.y.hou@… added
  • Cc sabreuse@… added

dd3220 months 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

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

dd3220 months ago

typo fix

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

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.

Still planned for 3.3?

Last edited 19 months ago by ocean90 (previous) (diff)
  • Milestone changed from 3.3 to Future Release
Note: See TracTickets for help on using tickets.