Feature/untangle criteria class

[mringler]

I was in the mood for some refactoring, and decided it is time to go for the white whale, aka the Criteria classes.

I believe those are Propel's achilles heel, since they are the center classes of ActiveQuery and parent class to all Query classes, and completely unmaintainable - with together almost 6000 lines of code, a fuzzy extension hierarchy, and them doing literally dozens of things at once. In practice, that is acceptable, as long as everything works perfectly. But almost all bugs I have fixed were in those three classes, and I am pretty sure there are more. Also, I am pretty sure that a lot of performance is lost by hacky workarounds, of which I have seen plenty. I think those bugs and workarounds show that the information flow in those classes is not understandable anymore.

So here is a proposal to start getting things back in order by removing sql query generation and execution out of those classes. This already removes more than 500 lines of code.

The basic idea is to have the doSelect(), doInsert(), do...() methods delegate to dedicated classes. Here is an overview of the new classes (not sure if the tree output renders in all browsers):

ActiveQuery
├── BaseModelCriteria.php
├── Criteria.php
├── ModelCriteria.php
├── ...
├── QueryExecutor
│   ├── AbstractQueryExecutor.php
│   ├── CountQueryExecutor.php
│   ├── DeleteAllQueryExecutor.php
│   ├── DeleteQueryExecutor.php
│   ├── InsertQueryExecutor.php
│   ├── QueryExecutionException.php
│   ├── SelectQueryExecutor.php
│   └── UpdateQueryExecutor.php
└── SqlBuilder
    ├── AbstractSqlQueryBuilder.php
    ├── CountQuerySqlBuilder.php
    ├── DeleteQuerySqlBuilder.php
    ├── InsertQuerySqlBuilder.php
    ├── SelectQuerySqlBuilder.php
    └── UpdateQuerySqlBuilder.php

The changes are very thoroughly tested through the Criteria classes and a few new tests.

[mringler]

Hmm, tests fail before they even start:

Tests started in temp /tmp.
Could not read "tests/sqlite.phpunit.xml".
Error: Process completed with exit code 2.

I know it is what everybody says, but I don't see how this is my fault. I have not changed the phpunit.xml files.

Please let me know how to proceed.

[codecov-commenter]

Codecov Report

Merging #1747 (a70d586) into master (a8f3afa) will increase coverage by 0.03%. The diff coverage is 94.31%.

@@             Coverage Diff              @@
##             master    #1747      +/-   ##
============================================
+ Coverage     87.89%   87.93%   +0.03%     
- Complexity     7827     7870      +43     
============================================
  Files           267      281      +14     
  Lines         22546    22670     +124     
============================================
+ Hits          19817    19934     +117     
- Misses         2729     2736       +7     

Flag	Coverage Δ
5-max	87.93% <94.31%> (+0.03%)	:arrow_up:
7.4	87.93% <94.31%> (+0.03%)	:arrow_up:
agnostic	67.44% <71.76%> (-5.91%)	:arrow_down:
mysql	70.38% <92.71%> (+0.19%)	:arrow_up:
pgsql	70.33% <93.50%> (+0.20%)	:arrow_up:
sqlite	68.30% <91.33%> (+0.19%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...untime/ActiveQuery/Criterion/AbstractCriterion.php	89.10% <ø> (ø)
.../ActiveQuery/QueryExecutor/InsertQueryExecutor.php	81.25% <81.25%> (ø)
...tiveQuery/QueryExecutor/DeleteAllQueryExecutor.php	83.33% <83.33%> (ø)
...me/ActiveQuery/SqlBuilder/CountQuerySqlBuilder.php	84.00% <84.00%> (ø)
src/Propel/Runtime/ActiveQuery/ModelCriteria.php	95.31% <87.50%> (+0.48%)	:arrow_up:
src/Propel/Runtime/Adapter/Pdo/PdoAdapter.php	90.96% <92.30%> (-0.47%)	:arrow_down:
.../ActiveQuery/QueryExecutor/UpdateQueryExecutor.php	94.44% <94.44%> (ø)
...e/ActiveQuery/SqlBuilder/UpdateQuerySqlBuilder.php	94.64% <94.64%> (ø)
...e/ActiveQuery/SqlBuilder/SelectQuerySqlBuilder.php	94.66% <94.66%> (ø)
...e/ActiveQuery/SqlBuilder/InsertQuerySqlBuilder.php	96.15% <96.15%> (ø)
... and 31 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a8f3afa...a70d586. Read the comment docs.

[mringler]

Added test fixed from #1751. I'll remove them and rebase when that is resolved.

[dereuromark]

Small conflict btw.

[mringler]

This is probably not going to get merged, so I am just going to close the PR. Should you actually want to see this merged, let me know, and I'll fix the conflict and re-open.

[dereuromark]

From what I can see it cleans up the baseline quite a bit So it seems worth finishing this up IMO.

[mringler]

Great, I have fixed the conflict

[dereuromark]

@nederdirk can also maybe help with the review for this to be merged today?

[dereuromark]

I see 1-2 important comments, otherwise we can probably continue for the beta release. Lets see if we can merge this today then.

[dereuromark]

This one fail

$DB_USER not set. Using 'root'. /usr/bin/mysql Ver 14.14 Distrib 5.7.35, for Linux (x86_64) using EditLine wrapper

looks unrelated

[dereuromark]

Should this be a squash merge?

[mringler]

One of the mysql tests always fails, but I think it is a problem with the pipeline (it's a different one each time) https://github.com/propelorm/Propel2/pull/1747/checks?check_run_id=3302733292 https://github.com/propelorm/Propel2/runs/3302662191?check_suite_focus=true

[mringler]

Should this be a squash merge?

I'd prefer it