Pkp-lib: Remove ADODB

Created on 2 Oct 2020  路  8Comments  路  Source: pkp/pkp-lib

Remove ADODB.

Dev Housekeeping
Source
picture asmecher

All 8 comments

PRs:

TODO:

  • [x] Fix search engine cached keyword results feature (Fixed by removing feature -- suspect it was ineffective and would like to solve it separately, e.g. https://github.com/pkp/pkp-lib/issues/3304)
  • [x] Fix (or remove) "create database" installer option (createDatabase in lib/pkp/classes/install/PKPInstall.inc.php) (Opted at least for now for removal: https://github.com/asmecher/pkp-lib/commit/cdb0b949850ffcdbe25713cb4810bd0a9abc0c50)

Highlights/implications:

  • Result counts are no longer available automatically when you run a query (e.g. DAOResultFactory->getCount / DAOResultFactory->wasEmpty). It's always been inefficient to both iterate and count, and using Generators (as we will do more in the future) means it's a bad pattern. It's sometimes necessary, though, e.g. for paging.

    • For existing use cases, you can provide the SQL to the DAOResultFactory (e.g. EmailLogDAO::_getByEventType) to allow it to count with a separate query (ADODB used to do this behind the scenes anyway).

    • For future use cases, if counting query results is really needed, try Laravel's Lazy Collections.

    • You will see a DAOResultFactory instances cannot be counted unless supplied in constructor exception if you need to adjust this.

  • Retrieving DB results has changed now that ADODB's functions are removed.

    • The DAOResultFactory is still available and works roughly the same, though it should be considered deprecated.

    • Instead, for multiple results, use either

    • ...the patterns in place in the Service classes (which use iterators), or

    • ...you can experiment with Generators (e.g. AnnouncementDAO::getByTypeId), which can be looped through using foreach but don't instantiate the objects it's fetching until they're needed, or

    • ...experiment with a new pattern with Laravel's Lazy Collections

    • For single results, the getters become really short and sweet (e.g. SubscriptionTypeDAO::getById for an object, and ContextDAO::existsByPath for single fields).

  • The DAO query functions (DAO::retrieve, DAO::retrieveLimit, DAO::retrieveRange, DAO::update) you're used to are still mostly present, but have some slight behavioural changes:

    • If you are only binding a single parameter to a query, you now need to put it in an array, where ADODB would accept either a literal or an array. (See e.g. ContextDAO::getByPath.) You will see Argument 1 passed to Illuminate\Database\Connection::prepareBindings() must be of the type array if you need to adjust this.

    • $dao->update now returns a count of affected rows, whereas before it returned a success/failure boolean that was frequently misused and undocumented. I have not cleaned up all these cases. See e.g. TemporaryFileDAO::deleteTemporaryFileById. Error conditions should use Exceptions rather than null/false returns that frequently go uncaught.

    • DAO::retrieveCached has been removed pending a suitable replacement (probably a different approach than a query-level cache).

    • However, these functions should be considered deprecated in favour of Laravel's query builder tools. You can either use the patterns set in the query builder classes (these will likely be changed soon) or use the Capsule object to construct queries directly in the DAO. See e.g. AnnouncementDAO::getById.

  • The ADODB XML schema management toolset has been removed. This includes the removal of tools/dbXMLtoSQL.php.
asmecher picture asmecher on 2 Oct 2020

@NateWr, I've finally got this passing tests; could you take a preliminary review? There are notes on the coding implications (and links to the PRs) at https://github.com/pkp/pkp-lib/issues/6264#issuecomment-702926627 (these should be adapted into the release notebook).

I'm OK to wait to merge this until after your submission file stuff is done, if that makes life easier.

asmecher picture asmecher on 6 Nov 2020

Looks really good @asmecher! I've done a review of those PRs with a few comments but nothing major. Thanks for the helpful guide to the changes in your comment. :+1: I've got a running list for the release notebook and I've added your comment to that.

NateWr picture NateWr on 10 Nov 2020

Thanks, @NateWr! I've run through the issues you flagged and fixed some minor ones, flagging a couple bigger ones with specific commits to resolve them. If you're OK with the result, next step will be to port to OMP and OPS, but I suggest holding off on that until you get the submission file stuff merged; then I'll rebase my OJS work on top of that before porting to the other apps.

asmecher picture asmecher on 11 Nov 2020
👍1

Great! Looks good to me. If that lastInsertId thing is ok as-is, I can have the submission file stuff merged tomorrow.

NateWr picture NateWr on 11 Nov 2020

@NateWr, I've added pulls for OPS and OMP to https://github.com/pkp/pkp-lib/issues/6264#issuecomment-702926627 -- could you have a quick look at these? They're essentially straight adaptations of the OJS PR (that is to say, lots of DAO class clean-up and little else).

asmecher picture asmecher on 18 Nov 2020
👍1

All PRs merged, not including the testing-purposes-only commits.

asmecher picture asmecher on 19 Nov 2020
🎉1

Closing this; there will doubtlessly be related errors but they can either reopen this issue with details or file new entries.

asmecher picture asmecher on 20 Nov 2020
Was this page helpful?
0 / 5 - 0 ratings

Related issues

mhvezina picture mhvezina  路  7Comments
asmecher picture asmecher  路  3Comments
NateWr picture NateWr  路  5Comments
mtub picture mtub  路  6Comments
ajnyga picture ajnyga  路  7Comments