Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(dev/core#4999) Convert xml schema to new entityType.php format #29472

Merged
merged 22 commits into from
Apr 19, 2024

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Feb 22, 2024

Overview

This overhauls the way schema entities are declared.
Implements plan from https://lab.civicrm.org/dev/core/-/issues/4999

Before

  • An entity type (e.g. "Activity") was declared in an xml file (e.g. xml/schema/Activity/Activity.xml).
  • The GenCode script was used to generate a DAO file (e.g. CRM/Activity/DAO/Activity.php) and install/uninstall sql files.
  • Note that the xml files themselves were never used at runtime, they were just the source of generated DAO & SQL files.

After

  • An entity type (e.g. "Activity") is declared in a php file (e.g. schema/Activity/Activity.entityType.php).
  • No code needs to be generated. Schema information is read directly from entityType.php files at runtime.
  • The xml files along with the generated sql files can be deleted.
  • DAO files can be gutted to an empty class extending CRM_Core_DAO_Base.

Copy link

civibot bot commented Feb 22, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Feb 22, 2024
@colemanw colemanw marked this pull request as draft February 22, 2024 15:07
@colemanw colemanw force-pushed the schemaEntity branch 3 times, most recently from 66cbee6 to 025c02c Compare March 3, 2024 03:37
@colemanw colemanw changed the title WIP - Add EntityRepository, EntityInterface WIP - Add EntityRepository, EntityInterface, DAO_Base Mar 14, 2024
@colemanw colemanw force-pushed the schemaEntity branch 3 times, most recently from db3e98e to de2bbf5 Compare March 20, 2024 00:38
@totten
Copy link
Member

totten commented Mar 25, 2024

@colemanw OK, so here are forays into synthesizing #29472 and #29422...

  • To generate civicrm-core:schema/**.entityType.php, I used Add convert-entity command totten/civix#342 and simply copied the schema-files to/from placeholder extension. Something like:

    cd sites/all/modules/civicrm
    (cd ext && civix generate:module placeholder)
    mkdir -p "ext/placeholder/xml/schema/CRM"
    cp -r xml/schema/* ext/placeholder/xml/schema/CRM/
    (cd ext/placeholder && civix convert-entity --core-style)
    cp -r ext/placeholder/schema schema
  • Branch A builds on those files. It also updates all references to CRM_Core_CodeGen_Schema and uses SqlGenerator instead. So it generate core-core schema from *.entityType.php

    • I wanted to see that the new SQL output matches the old SQL output. The basic idea is
      cp sql/civicrm.mysql sql/civicrm.mysql-orig
      ./bin/setup.sh -g
      diff -u sql/civicrm.mysql-orig sql/civicrm.mysql
    • However, the immediate issue is that setup.sh will fail because the table-sort algorithm doesn't work with core tables. (This issue would affect any call path; it's just easier QA in this flow.)
      Parsing schema description schema/Schema.xml
      Extracting database information
      Extracting table information
      Generating civicrm.config.php
      Generating table list
      Generating sql file
      
      Fatal error: Uncaught MJS\TopSort\CircularDependencyException: Circular dependency found: Activity->Phone->Contact->Contact
              in .../modules/civicrm/vendor/marcj/topsort/src/CircularDependencyException.php on line 34
      MJS\TopSort\CircularDependencyException: Circular dependency found: Activity->Phone->Contact->Contact
              in .../modules/civicrm/vendor/marcj/topsort/src/CircularDependencyException.php on line 34
      
  • Branch B builds on Branch A. It moves the SqlGenerator to be part of civimix-schema's SchemaHelper. And it still uses that for for core generation.

    • This works fails in the same way. So it loads+uses the code from civimix-schema to initialize core-core. But SqlGenerator dies on the same circular dependency.

For comparison, these are the sort-algorithms floating around:

@colemanw
Copy link
Member Author

colemanw commented Mar 25, 2024

@totten I think there might be some actual circular references in our schema unfortunately. 🙈

But looking at the error message you shared it doesn't look like a circle (dependency found: Activity->Phone->Contact->Contact), so 🤞🏻maybe🤞🏻 it's a simple matter of doing this?0238b45

@colemanw colemanw force-pushed the schemaEntity branch 2 times, most recently from ee4a4e7 to 0bed59e Compare March 26, 2024 01:35
@totten
Copy link
Member

totten commented Mar 26, 2024

@colemanw OK, updates:

  • Rebased and combined with master-phpsql-b.

  • Extracted a couple small PRs: (NFC) Contribute/Product.xml - No newline in comment #29825, (NFC) gitignore - Don't care about .composer-downloads #29826

  • We'll probably want to re-run convert-entity before a final merge. But it's a lot easier to test the branch if we have some live schema files.

  • Can currently run ./bin/setup.sh -g and view reasonable-looking output (sql/civicrm.mysql).

  • I've included fixes for several things -- but the generated SQL still doesn't quite run yet.

  • A decent way to test is like:

    ./bin/setup.sh -g && amp sql -r ~/bknix/build/dmaster/web/ -Ncivi <  sql/civicrm.mysql

    (After we get that working, we can look to cv core:install...)

  • Right now, it fails for me with:

    ERROR 1830 (HY000) at line 2729: Column 'case_type_id' cannot be NOT NULL: needed in a foreign key constraint 'FK_civicrm_case_case_type_id' SET NULL
    

@totten totten force-pushed the schemaEntity branch 2 times, most recently from 8f71d49 to f075d19 Compare March 27, 2024 00:54
@colemanw colemanw changed the title WIP - Add EntityRepository, EntityInterface, DAO_Base Convert xml schema to new entityType.php format, remove generated DAO & sql files Apr 16, 2024
@colemanw colemanw marked this pull request as ready for review April 16, 2024 20:45
Add new low-level class to gather schema metadata
Work toward getting rid of xml-based schema files in favor of entity.php files.
@totten
Copy link
Member

totten commented Apr 19, 2024

There's an extra set of test-runs attached to the last commit... to confirm that tests will generally work the same in other environments. We encountered one issue which should (already) be fixed. The latest run looks OK. (The extra tests aren't green.... but they are consistent with baseline. PR supplement,baseline)

@colemanw
Copy link
Member Author

Everything's passing & the additional test run didn't turn up anything.
Whew! This was a big endeavor!

@colemanw colemanw merged commit 07e57e8 into civicrm:master Apr 19, 2024
3 of 4 checks passed
@colemanw colemanw deleted the schemaEntity branch April 19, 2024 01:52
@totten
Copy link
Member

totten commented Apr 19, 2024

Hail, Github, full of grace,
the Testing is with thee.
Blessed art thou amongst data-stores
and blessed is the fruit of the keyboard, CiviCRM
Holy Github, Mother of Patches,
pray for us mergers,
now and at the hour of our commit.
Amen.

@colemanw
Copy link
Member Author

Amen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants