Add doctrine/dbal as substitute for ADOdb

[gohanman]

This alters to SQLManager to

1. Create connections using doctrine/dbal if present
2. Use those connections for all functions

It can be disabled by configuration. Those checks aren't currently applied correctly so that CI tests can run, but if this gets merged those will be changed so it defaults to using ADOdb as normal.

This could someday solve the problem of bundling in a pretty old copy of ADOdb.

I did remove a couple functions, affectedRows and dataSeek, each of which had literally one use. Altering those spots seemed easier than finding a way to make doctrine mimic those functions.

[lgedgar]

I was gonna try this out just for fun but can't do composer install on it using Composer v1. I see that "plugin-api-version" was bumped to 2.1.0 in composer.lock - does this mean Composer v2 is now required? And if so is that any problem for older installs etc.

[lgedgar]

Sorry I see now that is a result of https://github.com/CORE-POS/IS4C/commit/0c3ad8186c1bf6e4b3f2d8ed02a147f716694061 so not part of this PR...

[gohanman]

Yeah, that could have been communicated better. Finding a version of the newer library for #1093 that would install with Composer v1 was a bunch of extra work, and the steady stream of deprecation warning while doing so made me think it'd be worthwhile to resolve the issues keeping us stuck on v1.

I think existing installs should be able to just do composer install w/ v2. Now that the lockfile has all v2-compatible package versions there shouldn't be any of the circular dependency headaches related to composer itself.

[lgedgar]

Using v2 seems to be fine. Very basic initial usage of new DB layer yielded no surprises FWIW although I'm sure it'd be the edge cases if there were problems. Cool to see doctrine in the mix.

[lgedgar]

Er, turns out there were warnings. If it helps here they are:

[2021-09-23 16:32:52] fannie.WARNING: Undefined variable: params Line 616, /srv/corepos/upstream/fannie/IS4C/common/SQLManager.php [] []
[2021-09-23 16:32:52] fannie.WARNING: Undefined variable: which_connection Line 616, /srv/corepos/upstream/fannie/IS4C/common/SQLManager.php [] []
[2021-09-23 16:32:52] fannie.WARNING: Failed Query on /upstream/admin/labels/ShelfTagIndex.php              SELECT superID,                 super_name             FROM MasterSuperDepts AS m             WHERE superID > 0             GROUP BY superID,                 super_name          An exception occurred while executing '             SELECT superID,                 super_name             FROM MasterSuperDepts AS m             WHERE superID > 0             GROUP BY superID,                 super_name         ':  Prepared statement needs to be re-prepared  [] []

[lgedgar]

I don't see those warnings anymore FWIW.

[lgedgar]

Looking through this again and wondering, would it be safe (enough) to merge as-is? I don't grok the details enough to know for sure but if Doctrine can effectively be opt-in-only and otherwise legacy code runs, then presumably that's safe enough?

I had dreams of using Doctrine for its ORM someday, not sure how feasible that is, but if it can obviate the bundled ADOdb then maybe that's reason enough to pursue this. Currently the demo I'm working on could be opted in to test things out, to the extent a demo is really tested.

[gohanman]

In theory it should be pretty safe as long as doctrine isn't installed and highly experimental if it is. It probably also raises a question if what should be in git is something like composer.json.default & composer.lock.default to allow for local variation in what packages are installed.

I don't have a lot of time to work on this at the moment, but I'm open to revisiting it later this year.

[lgedgar]

I have "strong opinions" about this, not sure how well-informed, but they are:

• composer.json and composer.lock should remain in main repo (do not use .default "templates")
  • https://getcomposer.org/doc/01-basic-usage.md#commit-your-composer-lock-file-to-version-control
  • https://getcomposer.org/doc/02-libraries.md#lock-file (not applicable since CORE is not a "library" ?)
• forking should mean someone is willing to shoulder the burden it implies, e.g.
  • upstream has no responsibility to ensure they can "smoothly" merge any future changes, but esp. for composer.lock
  • https://getcomposer.org/doc/articles/resolving-merge-conflicts.md
  • hence forking should be "discouraged" or at least "less encouraged" than it has been
• with this PR then, Doctrine would "always" be installed since it's listed in composer.lock
• so maybe instead of checking for its existence, a "feature flag" setting should be enabled to use it

To me that seems safe enough but maybe that's just me..?

Agreed though, "local variation in what packages are installed" is basically an unsolved issue since there are several forks already in the wild. Many of these have differing composer.lock though I suspect not always for good reasons (e.g. user might run composer update in fork when it's not actually needed). Even if Poser (#1074) is used, not clear to me if/how extra dependency packages can be installed to support custom plugins. Maybe the Poser dir needs its own separate composer.json and must do a separate autoload call to leverage its separate vendor dir?

Anyway no worries for now, this can wait. I look forward to solving to the extent we can though.

[lgedgar]

See also #1185 for ideas on Composer management.

(Nothing more to add for now - obviously this can wait - just wanted that reference in there.)