Issue 313: Create new connector for Theme Editor #341
Conversation
| return; | ||
| } | ||
|
|
||
| if ( ! isset( $_POST['action'] ) || 'update' !== $_POST['action'] ) { |
There was a problem hiding this comment.
@powelski Nice opportunity to use wp_stream_filter_input() here:
'update' !== wp_stream_filter_input( INPUT_POST, 'action' )And elsewhere throughout this method...
| @@ -312,7 +312,7 @@ public static function callback_pre_set_site_transient_update_plugins( $value ) | |||
| } | |||
|
|
|||
| public static function callback_wp_redirect( $location ) { | |||
| if ( ! preg_match( '#(plugin|theme)-editor.php#', $location, $match ) ) { | |||
| if ( ! preg_match( '#(plugin)-editor.php#', $location, $match ) ) { | |||
There was a problem hiding this comment.
@powelski Shouldn't we then also include an update routine to fix past entries? Otherwise we will not be maintaining proper backwards compatibility. The old entries should be changed to the new Theme Editor connector with the proper context being the theme name.
There was a problem hiding this comment.
@fjarrett We can update summaries, but it's not possible to provide action links as I did here, because we don't have theme's slug in meta, only the name. Unless we do trick, like fetching available themes and adding theme slug meta for all matches. Not perfect, but worth consideration.
There was a problem hiding this comment.
@powelski I'm more concerned about just updating the Summary, Connector and Context more than the action links.
There was a problem hiding this comment.
@fjarrett Okay, I developed the updater. I tested it carefully. This how it works:
- It updates summary, connector, context and action for all previous file editions
- It tries to find a theme by name and if one is found, theme slug is saved in meta
- If we have theme slug in meta, we are able to display action links: Edit File and Edit Theme. Otherwise they are not present.
What I need to confirm is that connectors classes are available when WP_Stream_Install::update() is called (https://github.com/x-team/wp-stream/blob/issue-313/includes/install.php#L207-L211), because I tested it by calling Editor's updater directly.
There was a problem hiding this comment.
@powelski This is why we introduced the wp_stream_after_connectors_registration filter and place all migration methods directly inside the WP_Stream_Install class.
We should use this same approach and keep all migration logic in the same place.
There was a problem hiding this comment.
@fjarrett So you mean that this functionality should be moved from Editor's class to updater directly, is this correct?
There was a problem hiding this comment.
@powelski To the WP_Stream_Install class, yes. And we can run it after all connectors have registered.
|
@fjarrett |
| @@ -201,7 +201,13 @@ public static function update( $db_version, $current ) { | |||
| // If version is lower than 1.3.0, do the update routine for site options | |||
| // Backward settings compatibility for old version plugins | |||
| if ( version_compare( $db_version, '1.3.0', '<' ) ) { | |||
| add_filter( 'wp_stream_after_connectors_registration', 'WP_Stream_Install::migrate_old_options_to_exclude_tab' ); | |||
| add_action( 'wp_stream_after_connectors_registration', 'WP_Stream_Install::migrate_old_options_to_exclude_tab' ); | |||
There was a problem hiding this comment.
Oh noes! That means the migration of old options wasn't working. Could have been the culprit for this forum thread.
Issue 313: Create new connector for Theme Editor
Fix #313