Fixes #7276 display update progress in core:archive command (includes Updater refactor) #7333
Conversation
…(moved CoreUpdater static functions to core/Updater class, remove use of static methods in Updater, don't use static method setUpdater in ColumnsUpdater).
…er in Columns\Updater and document methods in Updater.
…er update process and document UpdateListener.
…notice errors caused by change to Updates.php and add some more functionality to ConsoleCommandTestCase base class.
… core:update command.
… to show progress of update. Also remove CLI output code from CoreUpdater\Controller::runUpdaterAndExit.
diosmosis
added
Pull Request WIP
|
Instead of reimplementing an event listener (for the "updater observer") would it make sense to reuse the event dispatcher instead? |
|
Do you mean create a new EventDispatcher for the Updater instance and post events to the member dispatcher? Or use |
|
I meant use Piwik::postEvent with either Piwik::addAction or the regular way ( |
|
Piwik::postEvent will post to every plugin, and the "events" executed by the command should only be executed for a single Updater run (ie during the execution of the command, and nowhere else). Also, Piwik::addAction additions cannot be removed, and the Updater is not a singleton service (at the moment) so posting global events from it would be unseemly. Note: Though this pull request has an 'observer' which seems akin to event listener, the intent is not to add an event dispatcher. The purpose of an event dispatcher is to provide extensibility, ie, the ability to create extensions. The purpose of the observer in this pull request is to allow Aspect Oriented Programming, so cross-cutting concerns can be added/removed non-intrusively on a case-by-case basis. |
mattab
added
the
not-in-changelog
|
(adding |
…php methods instance methods, create Update instances via DI, make Columns\Updater use instance methods instead of static, and add integration test for Columns\Updater.
…tController to FrontController.
diosmosis
removed
the
Pull Request WIP
|
@piwik/core-team I'm done w/ this change. Since it includes modifications to the Updates.php + descendents API, it should be reviewed. No BC breaking changes. |
| @@ -237,7 +236,7 @@ public static function wasDimensionMovedFromCoreToPlugin($name, $version) | |||
| 'log_visit.config_realplayer' => 'TINYINT(1) NOT NULL', | |||
| 'log_visit.config_device_type' => 'TINYINT( 100 ) NULL DEFAULT NULL', | |||
| 'log_visit.visitor_localtime' => 'TIME NOT NULL', | |||
| 'log_visit.location_region' => 'char(2) DEFAULT NULL1', | |||
| 'log_visit.location_region' => 'char(2) DEFAULT NULL1', // TODO: some of these have "NULL1", is this a bug? | |||
There was a problem hiding this comment.
@tsteur can you confirm whether it is expected that some of these have NULL1 ?
There was a problem hiding this comment.
yes, confirmed this already a few months ago. It's expected
There was a problem hiding this comment.
Do you remember why they are set to NULL1? Would be useful in a comment in the code.
|
Nice PR! I put some feedback in line. also, I found a bug which is related, but i guess it's not a regression of this Pull request. But because it's related it makes sense to solve it in here. How to reproduce a critical error (i guess there are way simpler technique but I did it this way)
Issues to address
Expected to get: 1 (error)
|
Conflicts: core/Console.php tests/PHPUnit/Framework/TestCase/ConsoleCommandTestCase.php
|
@mattab Addressed your comments (except for NULL1, will look for something to put as a comment later). |
|
Looks good to me (needs a rebase + you can merge it) |
Conflicts: plugins/CoreUpdater/Controller.php
…ally by catching them and re-throwing them.
Conflicts: plugins/CoreUpdater/Commands/Update.php
Conflicts: CHANGELOG.md
Fixes #7276 display update progress in core:archive command. Includes Updater refactor: deprecate static methods in Updates in favor of instance methods that are passed an Updater instance, use instance methods in Updater, move methods in CoreUpdater that perform updates to Updater class, add observer class that can be used within Updater to hook into single updater run.
Non-intrusively injects logic into the update process so the core:archive command will print out the current query being executed and the amount of queries left to run. Refactored Updater class + Columns\Updater class to use instance methods where possible, changed the methods of Updates.php base so Updater instance is passed to the descendants (preserved old method for BC). Added UpdateObserver class that can be used to inject logic into the update process. And moved static methods in CoreUpdater.php to Updater class.
Fixes #7276
TODO:
Command output looks like: