CRM-14885 - Commit DAO's. Add freshness test.

[totten]

Based on discussion from Mattermost, this PR adds DAO files to git. To
ensure that the files are kept up-to-date, it also adds a new test class
(CRM_Core_CodeGen_FreshnessTest).

The freshness test works by adding a checksum to the comments of each DAO.
The checksum is compared against (a) the source code (PHP/tpl) for GenCode
and (b) the XML schema for the particular table. (Update) The checksum is compared against the XML schema for the particular table. Additionally, the freshness test prepares a raw, unformatted version of the DAO file and compares it to the actual DAO file (modulo whitespace).

A few workflow notes for devs to be aware of:

• If you add or modify an XML schema file, be sure to run GenCode and
  commit the updated DAO. (If you neglect to do so, the test suite will
  report an error.)
• If you remove an XML schema file, be sure to remove the DAO file. (If
  you neglect to do so, the test suite will not report an error.)
• If you change any of the logic or templates behind GenCode, then all the
  DAOs need to be regenerated (even if the change has no functional impact
  on DAOs). This is a coarse-grained approach. Looking forward to the
  lifecycle (as devs tweak/refactor this code), this seems simpler/safer
  than trying to develop (or make mistakes with) fine-grained checks.
• (Update) You should not modify DAO files directly. (If you're unaware of this or rule and make a substantive change anyway, the test suite will report an error. However, this only protects against casually mistakes. If you're a trouble-maker, you can engineer whitespace changes that would go undetected. Don't do that. :P )

(Note: I drafted a patch to replace PHP_Beautifier with the slower but more correct phpcbf, but that would affect typical development dependencies. https://gist.github.com/totten/e76ff4d7243002322ca0d8e49924b5d1 )

---

• CRM-14885: Improve the workflow related to the GenCode.php script

[eileenmcnaughton]

Nice! Test fail is unrelated (I'll look into it).

It might be worth fixing these patterns before we commit - ie. the &import - I know these threw up so many errors when Chris ran a php7 over the codebase it rendered the tool fairly useless.

• static function &import($prefix = false) {
• if (!(self::$_import)) {

Also the caching of a statically declared array is dumb.

Does the test protect against someone editing the DAO & not the xml?

[totten]

+1 for removing the &. (Note: Since you mentioned PHP 7, I double-checked #8455 and that doesn't to seem to overlap.) And the caching is a bit silly - although I'm leaning toward-bug-level compatibility on the $prefix handling.

The test does not protect against someone editing the DAO directly. :( The only way to do that would be to have the test actually generate the full DAO and a comparison of the output.

[eileenmcnaughton]

"The test does not protect against someone editing the DAO directly. :( The only way to do that would be to have the test actually generate the full DAO and a comparison of the output."

Does this mean that becomes the job of human eyes to ensure? What comms do we need? It would be good - but not necessarily worth the effort - for a github hook to add a standard message to any PR that alters xml or DAO files

[totten]

Rebased. Patched DAO::needsUpdate() to use the new heuristic discussed on Mattermost.

[eileenmcnaughton]

From a QA point of view - assuming the $method() stuff is going to survive this round I feel the tests give a pretty good sanity check. Aside from that I'll pull this PR & do 'my normal dev' on top of it and also try rebasing it into a forked repo & see how that looks

[eileenmcnaughton]

test this please

[totten]

Slimmed down export() / import(). (Note: For the last two commits, I had to rebase/squash because my prior commit didn't handle the reference validly.)

[totten]

Random exciting thought -- after we commit more of these generated files, and after some time has passed, we'll be able to run git bisect without running GenCode. Weehoo!

[totten]

Hrm... all the build processes already run GenCode before running tests... which means that FreshnessTest will always pass, even if you commit stale data... we would need to skip GenCode as part of the test setup...

[eileenmcnaughton]

@totten do you have any reservations about this? I'm feeling good to merge it. It's a huge commit so there is a lot to look at but the DAO changes seem fine & I had a quick look at the cache change - it all seems like stuff that would have killed the tests if it were a problem.

I'll tinker a bit more but I'm feeling towards merging

Note I see you reluctantly didn't us REPLACE or INSERT ON UPDATE - my experience is they are often actually slower anyway....
"+ // This table has a wonky index, so we cannot use REPLACE or

• // "INSERT ... ON DUPE". Instead, use SELECT+(INSERT|UPDATE)."

[eileenmcnaughton]

I'm getting an error on download & install civisualize (no guarantee it is related to this PR)

[eileenmcnaughton]

[eileenmcnaughton]

test this please

[eileenmcnaughton]

(will try to see if I can install on test box)

[eileenmcnaughton]

@totten I experienced the same error as above on http://core-8769-5bct0.test-ubu1204-5.civicrm.org/civicrm/admin/extensions

[eileenmcnaughton]

I'm seeing another symptom on this sandbox that I'm NOT seeing on dmaster sandbox. I suspect it's the same underlying issue and sheds like on it.

If you go to

http://core-8769-5bct0.test-ubu1204-5.civicrm.org/civicrm/contact/dedupefind?reset=1&rgid=4&action=preview

& click continue you get a fatal. The reason for the fatal is that the rgid is NOT in the url that it re-directs to and the reason for that is that although in

CRM_Contact_Form_DedupeFind::preProcess we see

  /**
   * Pre processing.
   */
  public function preProcess() {
    $this->rgid = CRM_Utils_Request::retrieve('rgid', 'Positive', $this, FALSE, 0);
  }

When you get to PostProcess

$this->rgid
``` is empty

[totten]

@eileenmcnaughton , thanks for catching it! I cherry-picked your patch and added a test case (64c2ecd)

[eileenmcnaughton]

@totten I feel OK to merge this on pass now - that was one of very few places outside the DAO where the code changed at all & I haven't hit any other problems.

[totten]

@eileenmcnaughton Agree with merging on pass.

There is a caveat that this PR doesn't entirely meet the original goal -- e.g. in the current CI configuration, the FreshnessTest won't actually fail if you commit stale DAOs. But it still safe because all our CI and release scripts are designed to run GenCode anyway.

Despite the caveat, it's still a significant improvement because running GenCode will be more efficient. In the typical case where all DAO files are up-to-date, GenCode will finish much more quickly. (No need to call PHP_Beautifier.) In an atypical case where one DAO is stale, GenCode will only update that one DAO.