Issue 379 #387

c3mdigital · 2014-04-04T00:39:49Z

Fix #379
Fix #388
@fjarrett Please test and review

- get_instance() of WP_Stream_Install instead of calling check() method

- Add constructor method to initiate class then call check(); - Added prompt_update() method to handle user controlled db update

- Added final code documentation to class methods

- Fixed correct $_REQUEST keys to check for during each stage of update routine

- Replace esc_html with esc_html_e

- added error_log() calls to test installing over multiple versions - fixed bug - move the update database button to the end of the function

- Moved the database user upgrade process outside of the plugin list table hook. 1st Update Database notice is still there but rest of routine lives on it's own.

…performed

- Stores current connector versions and only fires callback function when version changes

…oaded now

- correct wp_stream_update query arg

…aming convention

…db update" Reverting the screwy conflict merges This reverts commit 1fceb01.

…xxx as naming convention" This reverts commit 6b6d69d.

- Added filter for extensions call there own update routine - Moved update routines to separate file

frankiejarrett · 2014-04-04T18:39:01Z

includes/install.php

+	 * To add your own stream extension plugin database update routine
+	 * use the filter and return your version that updating from requires an update
+	 * You must also make the callback function available in the global namespace on plugins loaded
+	 * use the wp_stream_update_{version_number} version number must be a 3 character string number with no periods


@c3mdigital Would it be at least 3 chars or exactly 3 chars?

Just thinking about double digit patch releases, like 1.4.12, which I believe is compatible with the semver spec.

@fjarrett Actually it wouldn't matter as long as their callback function is the same

- Added filter for extensions call there own update routine - Moved update routines to separate file

c3mdigital · 2014-04-15T15:10:50Z

@fjarrett I just did the exact same steps above but this time downloaded version 1.1.2 and got the error message.

frankiejarrett · 2014-04-15T15:12:04Z

@c3mdigital OK good, so I'm not crazy.

- Remove early return to allow multiple version updates to run - return false early only if update function fails - update functions should always return version string unless fail then they should return error object or false

c3mdigital · 2014-04-15T15:47:52Z

@fjarrett The problem was the wp_stream_update114() function was returning true instead of the version number if the character set was already defined, https://github.com/x-team/wp-stream/blob/issue-379/includes/db-updates.php#L291

…issue-379 Conflicts: includes/db-updates.php

frankiejarrett · 2014-04-16T21:30:43Z

@c3mdigital Right now, it's required that the plugin be updated via the WP dashboard in order for the DB update available message to appear, which is possible thanks to update_activation_hook.

However, if you manually overwrite the plugin files (like via FTP) you then have to deactivate/reactivate the plugin for the message to appear. This could cause a problem if the DB schema were to change via an update but the user never knew an update was available.

Would there be anything wrong with running WP_Stream_Install::check() all the time?

c3mdigital · 2014-04-16T21:42:05Z

@fjarrett The check method already runs all the time on init.

frankiejarrett · 2014-04-16T21:43:11Z

@c3mdigital Oh I see that now. OK so I was mistaken about having to deactivate/reactivate. Going to test again now.

frankiejarrett · 2014-04-16T22:32:56Z

@c3mdigital OK the dialog works without the need to deactivate! Awesome.

However, the upgrade routines don't appear to be working:

Install v1.2.8
Change some checkboxes in Stream > Settings > General > Log activity for
Change some checkboxes in Stream > Settings > Connectors
Remove the plugin
Clone in the latest and checkout issue-379
Run the DB update when prompted
There should be settings that migrated to Stream > Settings > Exclude but it's all empty

c3mdigital · 2014-04-16T22:45:31Z

@fjarrett I see the issue. wp_stream_update_128() Never fires because version compare is only looking for less than https://github.com/x-team/wp-stream/pull/387/files#diff-0e76810e485377b380ecabaec99134abR243

If we change that to if ( version_compare( $db_version, $version, '<=' ) ) { Then the db function should run. I'll commit the change now.

- Make sure db update function runs when updating from version that has update routine function

frankiejarrett · 2014-04-16T23:04:05Z

@c3mdigital The upgrades are still not executing.

I don't think the version compare operator is the problem because the settings migrations were introduced in 1.3.0: https://github.com/x-team/wp-stream/blob/issue-379/includes/db-updates.php#L129

c3mdigital · 2014-04-16T23:14:26Z

@fjarrett I see. I thought that was from version 1.2.8. Let me do some testing but it looks like running the update on the admin_notices hook is too late to add the action to wp_stream_after_connectors_registration

c3mdigital · 2014-04-16T23:40:45Z

@fjarrett Can you test again. Since the update needs the connectors to be loaded I added a call to reload the class so the update can attach to the wp_stream_after_connectors_registration hook.

frankiejarrett · 2014-04-16T23:54:57Z

@c3mdigital Worked! /five

Conflicts: includes/admin.php stream.php

frankiejarrett · 2014-04-17T00:24:29Z

includes/db-updates.php

+
+	do_action( 'wp_stream_before_db_update_' . $db_version, $current_version );
+
+	if ( version_compare( $db_version, '1.3.2', '<' ) ) {


@c3mdigital Just wanted to check with you on this one, this version compare shouldn't be required, right?

/cc @lukecarbis

… but add a check to see if the column exists

Issue 379

Chris Olbekson and others added 27 commits April 2, 2014 19:37

Issue-379 Database update API

89e8c4c

- get_instance() of WP_Stream_Install instead of calling check() method

Issue-379 Database update API

2ae7220

- Add constructor method to initiate class then call check(); - Added prompt_update() method to handle user controlled db update

Issue-379 Database update API

522fbf8

- Added final code documentation to class methods

Issue-379 Database update API

681844d

- Fixed correct $_REQUEST keys to check for during each stage of update routine

Issue-379 Database update API

a958370

- Replace esc_html with esc_html_e

Issue-379 Database update API

b85794d

- added error_log() calls to test installing over multiple versions - fixed bug - move the update database button to the end of the function

Issue-379 Database update API

dbadcb6

- Moved the database user upgrade process outside of the plugin list table hook. 1st Update Database notice is still there but rest of routine lives on it's own.

merge conflicst

beede2f

Code formatting

32a89c8

Refinement of update and redirect routines when database updates are …

5179cd9

…performed

Added register_update_hook that can be used by connectors

ab11604

- Stores current connector versions and only fires callback function when version changes

Moved WP_Stream::instal() to fire on init. Needs Stream_Admin to be l…

1cd3129

…oaded now

Fixed bug with query_string matching

56c1fd6

Final bug fixes from testing - ready to send pr

83c5c65

- correct wp_stream_update query arg

Formating and white space

65a328a

Added documentation to the new install / update methods

ec61422

Improving output of admin alerts

7c6808a

Space police

c0ae219

Include text domain in localized string, remove extra space

24ea6ad

Code formatting fixes

5d22a73

More code formatting

bb0fd0b

Added method to return array of plugin versions that require db update

1fceb01

Merge remote-tracking branch 'origin/issue-379' into issue-379

c084cbf

Convert each database change to a static method using update_xxx as n…

6b6d69d

…aming convention

Revert "Added method to return array of plugin versions that require …

e6230bd

…db update" Reverting the screwy conflict merges This reverts commit 1fceb01.

Revert "Convert each database change to a static method using update_…

0a24ca7

…xxx as naming convention" This reverts commit 6b6d69d.

Class method to fire the appropriate update functions

5d4fcea

- Added filter for extensions call there own update routine - Moved update routines to separate file

frankiejarrett reviewed Apr 4, 2014
View reviewed changes

Class method to fire the appropriate update functions

10d8ffb

- Added filter for extensions call there own update routine - Moved update routines to separate file

c3mdigital mentioned this pull request Apr 4, 2014

Don't rely on plugin_basename( WP_STREAM_DIR ) to define the db versions option_name #388

Closed

update method should only return false if error updating

231b36e

- Remove early return to allow multiple version updates to run - return false early only if update function fails - update functions should always return version string unless fail then they should return error object or false

frankiejarrett and others added 3 commits April 15, 2014 23:09

Changing docblock descriptions that were misleading

4ade702

Merge branch 'issue-379' of https://github.com/x-team/wp-stream into …

5a9d80c

…issue-379 Conflicts: includes/db-updates.php

Weird conflict.

923c022

Changed version compare to less than or equal to

56b8f74

- Make sure db update function runs when updating from version that has update routine function

Reload WP_Stream_Connectors to run db update action if needed

73d4011

frankiejarrett added 5 commits April 16, 2014 19:01

Updating deprecated functions

712056a

Code formatting

f72869d

Change version compare operator back to lessthan only

03630f8

Tweak update prompt message

57ef26e

Merge branch 'develop' into issue-379

775e481

Conflicts: includes/admin.php stream.php

frankiejarrett reviewed Apr 17, 2014
View reviewed changes

frankiejarrett and others added 3 commits April 16, 2014 21:07

Merge branch 'develop' into issue-379

12af2a1

Remove version compare

ba8fcf0

… but add a check to see if the column exists

Store the database version in the network options

4fef084

frankiejarrett added a commit that referenced this pull request Apr 17, 2014

Merge pull request #387 from x-team/issue-379

d2f6e9b

Issue 379

frankiejarrett merged commit d2f6e9b into develop Apr 17, 2014

frankiejarrett deleted the issue-379 branch April 17, 2014 02:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 379 #387

Issue 379 #387

c3mdigital commented Apr 4, 2014

frankiejarrett Apr 4, 2014

c3mdigital Apr 4, 2014

c3mdigital commented Apr 15, 2014

frankiejarrett commented Apr 15, 2014

c3mdigital commented Apr 15, 2014

frankiejarrett commented Apr 16, 2014

c3mdigital commented Apr 16, 2014

frankiejarrett commented Apr 16, 2014

frankiejarrett commented Apr 16, 2014

c3mdigital commented Apr 16, 2014

frankiejarrett commented Apr 16, 2014

c3mdigital commented Apr 16, 2014

c3mdigital commented Apr 16, 2014

frankiejarrett commented Apr 16, 2014

frankiejarrett Apr 17, 2014


		do_action( 'wp_stream_before_db_update_' . $db_version, $current_version );

		if ( version_compare( $db_version, '1.3.2', '<' ) ) {

Issue 379 #387

Issue 379 #387

Conversation

c3mdigital commented Apr 4, 2014

frankiejarrett Apr 4, 2014

Choose a reason for hiding this comment

c3mdigital Apr 4, 2014

Choose a reason for hiding this comment

c3mdigital commented Apr 15, 2014

frankiejarrett commented Apr 15, 2014

c3mdigital commented Apr 15, 2014

frankiejarrett commented Apr 16, 2014

c3mdigital commented Apr 16, 2014

frankiejarrett commented Apr 16, 2014

frankiejarrett commented Apr 16, 2014

c3mdigital commented Apr 16, 2014

frankiejarrett commented Apr 16, 2014

c3mdigital commented Apr 16, 2014

c3mdigital commented Apr 16, 2014

frankiejarrett commented Apr 16, 2014

frankiejarrett Apr 17, 2014

Choose a reason for hiding this comment