Introduce replacement schema management toolset (replacing ADODB XMLSchema)

[asmecher]

Introduce replacement toolset for ADODB.

• [x] Core schema management for installation
• [x] Introduce migrations for core upgrades
• [x] Schema creation for plugins
• [x] Upgrades for plugins [not currently supported but will be necessary for future plugin schema changes] [Edit: This was already present but never used! Grep PLUGIN_UPGRADE_FILE.] [Also apparently never worked.]

[asmecher]

On tools: https://github.com/pkp/pkp-lib/issues/2163 has introduced Laravel's query builder. That framework also has a Migrations toolset (https://laravel.com/docs/5.4/migrations).

Looking at comparables, there's also the standalone Phinx migration toolset; there's some discussion of the complications of using Laravel migrations outside of the Laravel framework, and using Phinx instead, that's directly relevant to our needs: https://www.reddit.com/r/PHP/comments/4g9g3n/using_illuminatedatabase_outside_of_laravel/

[asmecher]

On migrations: The most difficult aspect of this is probably going to be flipping the upgrade process to something that doesn't require ADODB.

Suggestion: we start coding using migrations tools, invoked from the upgrade script using <code> elements (or a new useful shorthand element like that) and avoiding dependence on upgrade steps that depend on ADODB tools like schema XML syncs. Once that's been maintained for a few releases, we can cut off upgrade compatibility at the same time we ax ADODB. We can synchronize this with a major release where upgrade cutoffs might be expected anyway.

[asmecher]

@bozana, I'd love your input on this! Just brainstorming at the moment.

[asmecher]

Another contender, which (at a glance) has ADODB-like table definitions in XML: http://propelorm.org/ (plus some relevant Propel/Phinx discussion from a while ago)

[bozana]

I will do some research and thinking... :-)

[netsensei]

Hi,

We are in the process of migrating from MySQL to PostgreSQL. I was just testing if DB upgrades are working on PostgreSQL when I ran into issue #1793. This issue suggests patching the ADODB library which isn't ideal.

So, that got me thinking.

I was wondering: have you looked into Doctrine/DBAL?

https://www.doctrine-project.org/projects/dbal.html
https://www.doctrine-project.org/projects/doctrine-dbal/en/2.9/reference/introduction.html#introduction

Both Propel (http://propelorm.org/) and Laravel's Eloquent (https://laravel.com/docs/5.8/eloquent) (which offers a migration toolkit) are fully fledged ORM libraries. The Doctrine Project offers it's own ORM alternative aptly named "ORM" (https://www.doctrine-project.org/projects/doctrine-orm/en/2.6/index.html)

Looking at OJS, my assessment would be that moving towards a fully fledged ORM would certainly bring many benefits and simplifications, but it's definitely a big challenge as it would require major architectural changes.

DBAL might be an intermediate solution. It would allow OJS to isolate database related logic and provide a nicely abstracted API that can be used, without necessarily being shoe-horned into OOP models that represent and track database state.

Further making the case for DBAL:

Schema manager:
https://www.doctrine-project.org/projects/doctrine-dbal/en/2.9/reference/schema-manager.html

• Represent your schema in XML or YAML and use that to create databases
• createSchema() allows you to compare schema's and calculate SQL queries for an upgrade path.

Platforms: https://www.doctrine-project.org/projects/doctrine-dbal/en/2.9/reference/platforms.html#platforms

• Out of the box support for many different platforms including MS SQL, MySQL, MariaDB, PostgreSQL, SQLite, Oracle SQL

Transactions https://www.doctrine-project.org/projects/doctrine-dbal/en/2.9/reference/transactions.html#transactions

• PDO like transactions with rollback options and auto-commit mode

Query builder https://www.doctrine-project.org/projects/doctrine-dbal/en/2.9/reference/query-builder.html#sql-query-builder

• Abstracts how you write queries as chained PHP function calls (builder pattern). The benefit here is that these are then translated to platform specific SQL query statements.
• You can - of course - still pass a plain old SQL query as well: https://www.doctrine-project.org/projects/doctrine-dbal/en/2.9/reference/data-retrieval-and-manipulation.html#data-retrieval-and-manipulation
• Safe passing of values to SQL queries avoiding SQL injection and the likes through prepared statements.

Here's an example application I wrote that leverages DBAL for managing a MySQL to MS SQL migration: https://github.com/VlaamseKunstcollectie/tmssync

So, a potential approach might look like this:

Taking a stab at the OJS code, I'd reckon that classes\db\DAO.inc.php would be your point of entry where you'd hook DBAL up into such functions like update and retrieve. You're also going to have to look at classes\db\DBConnection.inc.php and classes\db\SQLParser.inc.php.

Basically, you would keep the function signatures in those classes intact, but you'd replace references to ADODB with DBAL. That way, you'd avoid touching the rest of the codebase at this point. Testing whether it works would be by yanking out the entire ADODB library and all related "require" or "import" statements and see how much breaks.

Finally, you want to gradually strip out OJS code that's doing what DBAL already offers out of the box such as creating prepared statements, sanitising SQL queries, etc.

Regardless, I'm aware that replacing ADODB is a fundamental change to implement. So, it's not something I expect to land anytime soon.

[asmecher]

@netsensei, thanks, that's a very thoughtful summary of a bunch of good options.

I'm not sure if you've encountered OJS's service classes yet (e.g. https://github.com/pkp/pkp-lib/blob/master/classes/services/PKPSubmissionService.inc.php), and their associated query builder classes (https://github.com/pkp/pkp-lib/blob/master/classes/services/queryBuilders/PKPSubmissionQueryBuilder.inc.php) -- these are new tools that are gradually displacing what used to be coded in the DAOs. Our goal is to gradually move queries over to the querybuilder pattern, then shift the additional elements like schema management and upgrade processes over to a migrations-based toolset. And then we can get rid of ADODB. This will be quite a long process, but aspects of it are already well underway!

[netsensei]

Okay! That's great news!

I haven't seen those service classes yet. Looks like you're currently leveraging illuminate\database. And I can see that these are only first steps as, for instance, PKPAuthorService doesn't get yet instantiated elsewhere, right?

Consolidating your database interfacing logic into service classes is definitely a big step forward. I also notice there's work being done moving towards a PSR-4 compliant DI service container support to manage dependencies which is another good thing.

Keeping tabs on this!

[asmecher]

Proposed approach Introduce Laravel (Illuminate/Database) Schema Builder toolset and Migrations to gradually replace ADODB XMLSchema (which we've been using since OJS 2.0.0).

The main install.xml and upgrade.xml scripts will continue to be used to coordinate installs/upgrades, but will be augmented to support the new toolset in addition to the old one.

At a much later point, when it's no longer necessary to support both ADODB-based and Migrations-based upgrades (i.e. when we are OK to cut off the long tail of upgrades and require an intermediary upgrade version), we can restructure install.xml and particularly upgrade.xml to use more of the Laravel toolset.

Timeline

• OJS 3.2.1 to 3.3.0: Continue to use ADODB for schema management/upgrades, but keep changes minimal to facilitate backporting.
• OJS 3.3.0: Replace installer's schema management with Schema Builder. All upgrades/schema changes will henceforth use migrations.
• ADODB-based upgrades (anything prior to 3.3.0, or perhaps 3.2.1 if back-porting is easy) can be gradually sunset with a hard cut-off TBD.

Plugins that use schema management will have the same window during which to adapt their schema descriptors. The PRs contain a tool (tools/xmlSchemaToMigration.php in pkp-lib) to convert these.

Work-in-progress pull requests

• https://github.com/pkp/pkp-lib/pull/5861/files
• https://github.com/pkp/ojs/pull/2744/files
• https://github.com/pkp/usageStats/pull/31 (Example for how to adapt a plugin from XML Schema to Migrations -- schema script created using lib/pkp/tools/xmlSchemaToMigration.php)

[NateWr]

This looks really great! I didn't see any upgrade migrations, just the install migration. Is that right? My understanding is that an upgrade would be added by adding a new line to the upgrades.xml file:

<migration class="...Migration_3_3" />

And then that migration class would just have an up() and a down() method:

class Migration_3_3 extends ... {
  up() {
    Capsule::schema()::table('users', function (Blueprint $table) {
      $table->string('email')->nullable();
    });
  }
  down() {
    Capsule::schema()::table('users', function (Blueprint $table) {
      $table->string('email')->nullable(false);
    });
  }
}

Is that about right?

[NateWr]

Also, I noticed there is an ability to "back out" of an upgrade when an error occurs. This strikes me as a Very Good Thing. In terms of dropped data, will there be some way to keep dropped data around temporarily? It'd be great if in the up() I could say "keep this data around until the full upgrade process runs successfully" and then if there's an error the down() method could draw on that. But then if there's no error that temp data would just get cleared out...

[asmecher]

@NateWr, re: https://github.com/pkp/pkp-lib/issues/2493#issuecomment-627855559, yes, that's roughly how a migration would go into upgrades.xml -- it'll still typically get listed in a <upgrade minversion="2.4.0.0" maxversion="3.0.0.9"> node (though it doesn't have to) just like our current <code> and <data> elements.

I'd like to do a little more reading about Laravel's structures here to see if we can name/organize migration files according to their idiom in case we want to adopt more of the trappings later (e.g. the artisan tools mentioned here). I'll probably move the schema creation migrations from classes/migration into classes/migrations/schema (to differentiate them from the upgrade ones, which would go into classes/migrations/upgrade, which would take the place of dbscripts/xml/upgrade).

Also, I noticed there is an ability to "back out" of an upgrade when an error occurs.

Yes, and I've added a rudimentary structure to take advantage of this -- Installer::executeAction calls down() on a stack of already-executed migrations before bailing. However, I'm still very much ambivalent about whether this will prove practical. Any migration that's more than trivial will be hard to reverse, and it's likely to be an area that doesn't get well tested unless we're able to introduce some tools/practices we don't currently have. I think the 3.2.1 to 3.3 upgrade scripts will be a fairly simple and helpful proving ground.

While I'm currently proposing to merge this stuff in for 3.3, there's a modified form we could get into 3.2.1: we take the parts of this that add the new tools, but exclude the parts that convert the existing schema over to use them. That way we could start using the new tools for 3.2.0 to 3.2.1 upgrade steps with much lower risk, and move our migration horizon forward a step. However, that still feels a little big to shoehorn into a .x release. Any opinion?

[NateWr]

However, I'm still very much ambivalent about whether this will prove practical.

Yeah I agree. I think it would be great to implement if we can but it will be pretty challenging to do so -- especially in the complex migrations where it would matter most (eg - versioning in 3.2).

a modified form we could get into 3.2.1

I'd lean towards introducing it in 3.3. I appreciate that it's more new stuff rather than modified stuff, which means it's less likely to introduce bugs. But I would also see it as something that requires more testing before 3.2.1 is released. Leaving it to 3.3 means that we have a smaller batch of changes to test out before the release.

[asmecher]

I'd lean towards introducing it in 3.3.

:+1: Yup, let's go with that, thanks.

[asmecher]

Merged for ojs, pkp-lib, and usageStats. Working on the other apps, though backwards compatibility should be present in the meantime.

[asmecher]

Committed for OPS and OMP.