Improved plugins update API and how plugins define and reuse schema migrations #10028

tsteur · 2016-04-11T03:42:57Z

Plugins can now more easily define updates by reusing some existing migrations. So far a couple of database migrations and one plugin migration exists. In the future we can add many more migrations.

I also renamed some existing API methods but kept BC even though it is for 3.0 as I don't want to break updates when it's easy to keep BC for a while.

In an update one can now implement getMigrations() method and generate migrations via the migrations factory instead of specifying SQL queries manually via return array('SELECT ... => errorCodesToIgnore). While we still can define custom SQL queries we can often reuse existing migrations and this way we don't have to check for correct error codes all the time etc.

For example:

public function getMigrations(Updater $updater)
    {
        // many different migrations are available to be used via $this->migration factory
        $migration1 = $this->migration->db->changeColumnType('log_visit', 'example', 'BOOLEAN NOT NULL');
        // you can also define custom SQL migrations. If you need to bind parameters, use `->boundSql()`
        $migration2 = $this->migration->db->sql($sqlQuery = 'SELECT 1');

        return array(
            // $migration1,
            // $migration2
        );
    }

It is easy for us to add new migrations. In the future we could add more features:

eg also define undo() methods for some migrations although this will very likely not happen in the next years.
The migrations could automatically detect whether an update is a major update or not.
Migrations could in the future provide a method like checkPrecondition to eg check whether all required conditions are met. Say there is an UpdateConfig migration and this migration could check whether the config file is actually writable before the update is performed.
At some point we could "merge" alter table statements across multiple updates. For example if multiple updates define a migration for log_visit we could automatically combine them in one big alter update for faster performance etc.
For eg plugin changes we could print commands to execute on the console such as ./console plugin:activate $pluginName and not only print SQL updates
...

Currently, the following DB migrations are available:

AddColumn
AddColumns
AddIndex
AddPrimaryKey
BatchInsert
BoundSql
ChangeColumn
ChangeColumnType
ChangeColumnTypes
CreateTable
DropColumn
DropIndex
DropTable
Sql

For an example see https://github.com/piwik/piwik/blob/update_migrations_2/plugins/ExamplePlugin/Updates/0.0.2.php

To add a new migration create a new class in Piwik\Updater\Migration. Eg in the Db or Plugin directory. I can imagine there will be in the future also a Config directory etc. Then add a method to the Factory in within that directory. By using the factory everything will go through DI. Then a method should be added eg to tests/PHPUnit/Integration/Updater/Migration/Db/FactoryTest.php (here we test whether factory returns correct instance and we check eg if correct SQL is generated) and MigrationsTest.php (here we actually test whether the migration works) .

All the classes under core/Updater/Migration are not API apart from the Factories and a Db class containing constants for error codes. Only the factories are API which allows us to change code for the migrations in the future.

tsteur · 2016-04-11T03:49:03Z

core/Updater/Migration.php

+ *
+ * @api
+ */
+abstract class Migration


I first used an interface but this would mean we cannot add any new features as every new addition of a new method would break API. By using abstract class we can add at least new method like checkPrecondition or isMajorUpdate.

tsteur · 2016-04-11T04:46:38Z

@sgiehl or @mattab I already "reviewed" and checked that nothing should break but would be still good to have someone roughly looking over it again.

…ttings api

* More documentation * Added some more tests

tsteur · 2016-04-13T01:27:48Z

FYI: I fixed the tests (UI tests had a segmentation fault)

tsteur · 2016-04-13T02:16:48Z

I'd like to merge this one tomorrow if possible

sgiehl · 2016-04-14T21:26:56Z

core/Updater/Migration/Db/ChangeColumnTypes.php

+
+        $sql = sprintf("ALTER TABLE `%s` %s", $table, implode(', ', $changes));
+
+        parent::__construct($sql, static::ERROR_CODE_DUPLICATE_COLUMN);


Shouldn't that be ERROR_CODE_UNKNOWN_COLUMN?

The duplicate error maps to 1060 and this is what we used to use see eg https://github.com/piwik/piwik/pull/10028/files/756655316f1e2cfc9678e7c110ca9ce003f2c9c9#diff-b3898c538dda4ef48012ac8ac8ba4c16L28 . It sounds valid to ignore the error you mentioned but not sure why it was never done

Hm, I see. But imho that doesn't make sense. Can a duplicate column error even occur if the column name isn't changed?

i'm changing it to unknown and will see what happens. It does definitely make sense. Probably 1060 was used once to rename a column and then copied for all further change column uses.

sgiehl · 2016-04-14T21:39:10Z

Had a rough look through the changes. Basic changes looks fine (besides the one comment), but I didn't have a look at all the changes to the update scripts. Could maybe have a closer look at those tomorrow. But I can do that after merging, aswell.

tsteur · 2016-04-14T21:42:07Z

I had a look over all the update migrations twice, it should be fine otherwise and can fix changes otherwise later if needed I reckon

…ctually renaming a colum

* refs #7983 let plugins add or remove fields to websites and better settings api * * Hide CorePluginsAdmin API methods * More documentation * Added some more tests * improved updates API for plugins * better error code as duplicate column cannot really happen when not actually renaming a colum Conflicts: core/Updates/3.0.0-b1.php plugins/CoreUpdater/Commands/Update/CliUpdateObserver.php

…10397) * improved ui and responsiveness * improve rss widget * commit changes for ui again, got lost after the last commit * fix more tests * restoring files * fix fonts * fix more tests * more test fixes * fix some system tests * fix tests * fix system and ui tests * fix updater tests * make a page as loaded once the callback is called * enable verbose * more verbose output * enable phantomjs debug flag * debug should be a phantomjs option * trying to fix installation tests * fixes #10173 to not compile css files as less * trying to minimize js/css requests to hopefully prevent random ui test fails * disable verbose mode * fix updater and installation * lots of bugfixes and ui tweaks * fix reset dashboard * various bugfixes * fix integration tests * fix text color * hoping to fix installation tests this way * cache css/js resources for an hour, should speed up tests and prevent some random issues * we need to avoid installing plugins multiple times at the same time when requesting resources * finally getting the colors right again * fix most tests, more tests for theme * use an h2 element for titles for better accessibility * fix headline color * use actual theme text color (piwik-black) * fix small font size was applied on all p elements * fix tests * now improving all the datatables * trying to ignore images for visitor log * Revert "trying to ignore images for visitor log" This reverts commit ad1ff72. * fix tests * fix we had always ignored a max label width * trying to fix file permissions * fix more file permissions * Improved plugins update API (#10028) * refs #7983 let plugins add or remove fields to websites and better settings api * * Hide CorePluginsAdmin API methods * More documentation * Added some more tests * improved updates API for plugins * better error code as duplicate column cannot really happen when not actually renaming a colum Conflicts: core/Updates/3.0.0-b1.php plugins/CoreUpdater/Commands/Update/CliUpdateObserver.php * fix DB field piwik_log_visit.location_provider too small (#10003) * fixes #9564 fix DB field piwik_log_visit.location_provider too small * use new plugins updater API * DB field piwik_log_visit.visit_total_actions too small (#10002) * fixes #9565 DB field piwik_log_visit.visit_total_actions too small * change type of some db columns that are too small * fix tests (#10040) Conflicts: plugins/CoreAdminHome/Menu.php plugins/Goals/Menu.php plugins/MobileMessaging/Menu.php plugins/SitesManager/Menu.php plugins/UsersManager/Menu.php tests/PHPUnit/System/expected/test_apiGetReportMetadata__API.getWidgetMetadata.xml * fix more file permissions * repair more file permissions * repair more file permissions * trying to make ui tests work again, the table was missing * fix some encoding issues * cross browser fixes and usability improvement * move back the config icon, need to find a better solution later * more cross browser fixes * bugfixes * fix ui tests * fix encoding issue * fix various issues with the ui tests when a test gets aborted * also skip this visitor log test when aborted * there were 3 css files that were loaded separately, merge them instead into one css * forgot to add the actual manifest * do not add manifest if custom logo is specified * load font css files first as it was before merging them into big css * fix link icon was not aligned anymore * minor fixes * setting it back to 4px * in popovers the font variable was always ignored and a different font loaded * forgot to update screenshots * fix remaining tests * this should fix an update error * added 3 new widgets system check, system summary and plugin updates * tweak new widgets content * no page reload when changing date or segment * in admin home show only enabled widgets * refs #10295 use getMockBuilder instead of deprecated getMock * fix some ui tests * fix various bugs * fix more tests * fix ui tests * add a space between loading image and loading message * fix docs so they appear on developer.piwik.org * improved documentation * introduce new Widget::renderTemplate method for consistency with controllers * remove no longer needed files * testing system fonts * fix strong was not really bold * more useful system summary * remove ubuntu font * fix most tests and removed most em elements * fix tests * fix headline was very thin * update submodule * update submodules * update submodule * fix failing ui tests * update submodules

tsteur added the Needs Review PRs that need a code review label Apr 11, 2016

tsteur added this to the 3.0.0-b1 milestone Apr 11, 2016

tsteur reviewed Apr 11, 2016
View reviewed changes

tsteur force-pushed the update_migrations_2 branch from fe9d41f to ceb4130 Compare April 11, 2016 04:47

tsteur added 3 commits April 12, 2016 05:50

refs #7983 let plugins add or remove fields to websites and better se…

14c3fd0

…ttings api

* Hide CorePluginsAdmin API methods

65ecc28

* More documentation * Added some more tests

improved updates API for plugins

7566553

tsteur force-pushed the update_migrations_2 branch 2 times, most recently from e6020e1 to 7566553 Compare April 13, 2016 00:39

sgiehl reviewed Apr 14, 2016
View reviewed changes

better error code as duplicate column cannot really happen when not a…

d51f8ba

…ctually renaming a colum

sgiehl mentioned this pull request Apr 17, 2016

fix DB field piwik_log_visit.location_provider too small #10003

Merged

tsteur merged commit 252148a into 3.x-dev Apr 18, 2016

tsteur deleted the update_migrations_2 branch April 18, 2016 20:26

mattab mentioned this pull request Oct 2, 2016

When a database table column is updated, detect automatically whether it's a major upgrade or not #10629

Open

mattab changed the title ~~Improved plugins update API~~ Improved plugins update API and how plugins define and reuse schema migrations Oct 5, 2016

mattab added Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. labels Oct 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved plugins update API and how plugins define and reuse schema migrations #10028

Improved plugins update API and how plugins define and reuse schema migrations #10028

tsteur commented Apr 11, 2016

tsteur Apr 11, 2016

tsteur commented Apr 11, 2016

tsteur commented Apr 13, 2016

tsteur commented Apr 13, 2016

sgiehl Apr 14, 2016

tsteur Apr 14, 2016

sgiehl Apr 14, 2016

tsteur Apr 15, 2016

sgiehl commented Apr 14, 2016

tsteur commented Apr 14, 2016


		$sql = sprintf("ALTER TABLE `%s` %s", $table, implode(', ', $changes));

		parent::__construct($sql, static::ERROR_CODE_DUPLICATE_COLUMN);

Improved plugins update API and how plugins define and reuse schema migrations #10028

Improved plugins update API and how plugins define and reuse schema migrations #10028

Conversation

tsteur commented Apr 11, 2016

tsteur Apr 11, 2016

Choose a reason for hiding this comment

tsteur commented Apr 11, 2016

tsteur commented Apr 13, 2016

tsteur commented Apr 13, 2016

sgiehl Apr 14, 2016

Choose a reason for hiding this comment

tsteur Apr 14, 2016

Choose a reason for hiding this comment

sgiehl Apr 14, 2016

Choose a reason for hiding this comment

tsteur Apr 15, 2016

Choose a reason for hiding this comment

sgiehl commented Apr 14, 2016

tsteur commented Apr 14, 2016