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

Add FKColumnName to fields metadata #328

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

colemanw
Copy link
Contributor

@colemanw colemanw commented Jan 3, 2024

This supports civicrm/civicrm-core#28827 by adding the extra bit of info it needs.

This supports civicrm/civicrm-core#28827 by adding the extra bit of info it needs.
colemanw added a commit to colemanw/civicrm-core that referenced this pull request Jan 3, 2024
Uses the update from totten/civix#328 to add FKColumnName metadata.
@colemanw
Copy link
Contributor Author

colemanw commented Jan 3, 2024

@totten I can confirm that this works... I used it to generate civicrm/civicrm-core#28846 and https://lab.civicrm.org/extensions/cividiscount/-/merge_requests/304

@totten totten merged commit 5ea1146 into totten:master Jan 25, 2024
1 check passed
@totten
Copy link
Owner

totten commented Jan 25, 2024

We had chatted briefly last week about some of the example diff's produced on other ext's . (I had been concerned about whether the dropped functions would affect compat.) Coleman pointed that (basically) this change doesn't cause that. Which was a good point...

I guess the dropped-function part points to a general issue around generate:entity-boilerplate, civicrm-core:dao.tpl, and civicrm-core:CRM/Core/DAO.php -- i.e. the extensions can get out-of-sync with the templates and the live framework.

  • As things stand, this juggling act (generate:entity-boilerplate vs dao.tpl vs civicrm-core-version-compatibility) has always been an concern/risk, and the onus has always been for downstream developers to pick sufficiently-compatible-combinations. We just don't notice it much. (Probably b/c DAO's don't change much? Tho I doubt there's much awareness of that dynamic?)
  • I sometimes think that these DAO's should work like Smarty -- ie read the XML and put the PHP into templates_c. Then you don't need to manually regen DAOs, and they'll always match the dao.tpl of the active civicrm-core. But I'm sure there's some issue with that.

Anyway, it does seem better for entity-boilerplate to provide the additional metadata here. (It probably does help folks who select a newer dao.tpl, and it probably doesn't hurt folks who select an older dao.tpl.)

@colemanw colemanw deleted the referenceColumns branch January 25, 2024 12:59
@colemanw
Copy link
Contributor Author

I sometimes think that these DAO's should work like Smarty

I sometimes think they shouldn't exist at all! They're nothing but boilerplate that could be replaced with generic functions which read directly from schema metadata in the xml files.

Also, there are growing use-cases for "virtual" entities that have a db table but no corresponding DAO file (ECK, CiviImport, and SearchSegments all generate such entities). And those all work!

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

Successfully merging this pull request may close these issues.

2 participants