WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#18289 reviewing task (blessed)

Direct link to plugin installation should have admin chrome

Reported by: nacin Owned by: nacin
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 (7)

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

Download all attachments as: .zip

Change History (24)

csixty43 years ago

Patch to provide requested functionality

comment:1 csixty43 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?

comment:2 SergeyBiryukov3 years ago

  • Keywords has-patch added

comment:3 dd323 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..

andrewryno3 years ago

comment:4 andrewryno3 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.

comment:5 goto103 years ago

  • Cc dromsey@… added

comment:6 andrewryno3 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.

andrewryno3 years ago

andrewryno3 years ago

comment:7 andrewryno3 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.

comment:8 andrewryno3 years ago

Refreshed patch.

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

andrewryno3 years ago

comment:9 andrewryno3 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.

comment:10 helenyhou3 years ago

  • Cc helen.y.hou@… added

comment:11 sabreuse3 years ago

  • Cc sabreuse@… added

dd323 years ago

comment:12 dd323 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

comment:13 dd323 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()...

dd323 years ago

typo fix

comment:14 nacin3 years ago

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

comment:15 dd323 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.

comment:16 ocean902 years ago

Still planned for 3.3?

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

comment:17 nacin2 years ago

  • Milestone changed from 3.3 to Future Release
Note: See TracTickets for help on using tickets.