Make WordPress Core

Opened 13 years ago

Closed 12 years ago

#18977 closed task (blessed) (fixed)

Make import.php dynamic

Reported by: jane's profile jane Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.3
Component: Import Keywords: has-patch
Focuses: Cc:

Description

Instead of hard-coding the importers list, have a tag on importer plugins (that can only be set by core/people with permission) to automatically generate the importer plugins list. That way if new importers are added to the plugin directory, they'll automatically appear in wp-admin instead of having to wait for the next release for inclusion.

Attachments (4)

18977.diff (6.0 KB) - added by nacin 12 years ago.
18977.2.diff (8.3 KB) - added by nacin 12 years ago.
18977.patch (8.3 KB) - added by dllh 12 years ago.
Adds is_array() check to request response
18977.2.patch (8.3 KB) - added by dllh 12 years ago.

Download all attachments as: .zip

Change History (17)

#1 @nacin
13 years ago

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

@nacin
12 years ago

#2 @nacin
12 years ago

  • Keywords has-patch added; 3.4-early needs-patch removed
  • Milestone changed from Future Release to 3.4

Should be noted that there is code to prevent the transient from being stored here. Just remove the false && to test storage.

The API returns the following data:

  • array of importers
  • each importer is an array, keyed by the importer slug
  • importer data is the title, the description, and the importer's ID when used for register_importer() provided it does not match the slug

Not completely tested, but no logic should have changed.

Also, we should send $wp_version and get_locale() back to the API endpoint so we open the possibility to send back targeted lists.

#3 @nacin
12 years ago

Noticed that the patch does not cache API failures (which get_theme_feature_list() does). So this needs a bit more work.

#4 @ramiy
12 years ago

  • Cc r_a_m_i@… added

#5 @navjotjsingh
12 years ago

  • Cc navjotjsingh@… added

#6 @ryan
12 years ago

  • Milestone changed from 3.4 to Future Release

#7 @dllh
12 years ago

  • Cc daryl@… added

#8 @nacin
12 years ago

  • Milestone changed from Future Release to 3.5

@nacin
12 years ago

#9 @nacin
12 years ago

  • Keywords commit added

#10 @dllh
12 years ago

  • Keywords commit removed

18977.2.patch can cache bad data if the remote request returns but contains bad data. For example, I changed the hostname to xapi.wordpress.org and wound up caching an error page in the transient, which resulted in an error (couldn't do foreach) on the import page. Since the hostname's hard-coded, this particular error's not likely, but say the web server returned a 500 and an error document.

So we need either to check for a 200 response or to check to make sure $popular_importers as populated by the request looks valid. I favor checking to make sure the response is an array over checking for a 200. For example, say somebody hosed up the output from the api so that it returned a 200 but not a serialized array. We'd encounter a similar issue to what I've tested above.

The forthcoming patch does an is_array() check and unsets $popular_importers if not true so that the fallback return at the end of the function is used in this case.

Removing the commit keyword so that my patch doesn't accidentally get committed unreviewed.

@dllh
12 years ago

Adds is_array() check to request response

#11 @nacin
12 years ago

Good idea, and looks great. Only thing would be that if we call unset( $popular_importers ), then we need to check if ( ! empty( $popular_importers ) ) rather than if ( $popular_importers ) to avoid a potential notice.

@dllh
12 years ago

#12 @dllh
12 years ago

I've added a new patch with the following modifications from the last:

  • Sets $popular_importers to false instead of unsetting it.
  • Checks for is_array( $popular_importers) rather than just testing for existence.

I think this fixes the potential notice issue and is more consistent/robust; that is, if we're checking for an array in the one place, we should do the same below. This safeguards against the unlikely case that something else (a plugin, say) writes some non-array garbage into the transient.

I suppose that if we wanted to be extra robust, we'd also check for $popular_importers['importers'], but maybe that's overkill.

#13 @nacin
12 years ago

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

In 22632:

Pull the list of popular importers from WordPress.org.

These are the importers we suggest on import.php, prompting the user to
install the relevant plugin for the import they want to go through.

If the API is inaccessible, it falls back to a hard-coded list that should
be kept sync'd with the API with each major version of WordPress. This API
enables us to add new importers between releases, as they are completed or
if services gain quick adoption. As a last resort, we can also temporarily
disable importers that are broken (due to API changes, for example).

The importer currently returns English strings (which are then run through
translate() for existing strings), but the locale is passed to the API,
allowing us to ship translated strings if we wish to be adventurous.

props dllh for the assist.
fixes #18977.

Note: See TracTickets for help on using tickets.