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 export command for managed entities and afforms #312

Merged
merged 10 commits into from
Nov 16, 2023

Conversation

colemanw
Copy link
Contributor

@colemanw colemanw commented Sep 30, 2023

Adds a useful command which exports Afforms and managed records, leveraging the APIv4 Export action which collects sub-records into a single .mgd.php file (or in the case of Afforms, .aff.php and .aff.html and .mgd.phps for the search displays).
The resulting file includes E::ts() around translatable strings.

For a full demonstration, try exporting an existing afform, e.g.:

cv en civi_campaign
cd ext/civi_pledge # or any empty-ish extension
civix export Afform afsearchCampaignDashboard

It will write out the 2 files for the Afform plus 3 files for the 3 embedded SavedSearches into your extension (in this example civi_pledge), plus the navigation menu item.

@colemanw
Copy link
Contributor Author

colemanw commented Oct 4, 2023

Thanks for adding the test @totten - I'm pretty unfamiliar with the civix test framework (or the civix framework in general, although hey I figured out enough to add a new Builder class!) so that test would have taken me a long time to figure out.
It passed!

@totten
Copy link
Owner

totten commented Oct 4, 2023

@colemanw Yeah, the test framework was added several years after the original development. 👀 But I did a big push last year to make it more agreeable.

I have to admit... the Builders kinda scare me now-a-days, but what you did is good+consistent. (The $upgrader convention used by *.up.php files feels easier to work with. But it's never been used for generate:* cmd.)

I've been thinking/wondering about how this is likely to evolve and how much to accommodate these bits. Main thoughts:

  • I find it easier to understand export by name than export by ID. (There are a few ways to go at that -- e.g. incorporate the +w parser from cv. Or probe the target entity, trying to match by ID or name. Or have multiple options)
  • It's interesting how different the bits for Afform-export and api4-generic-export.
  • It's pretty tempting to split these out as different/named approaches to exporting, eg
    ## civix export <exporter> <options>
    
    ## civix export api4 <entity> <id>
    civix export api4 SomeEntity 123
    
    ## civix export afform <name>
    civix export afform myFormName
  • It would be nice to have a "re-export" or "export-all" command. Basically: take all the exports that you've done before and re-run them. It allows a workflow like:
    1. Edit data in web UI. Test in web UI.
    2. civix re-export (or civix export --all or somesuch)
    3. git diff (If you don't like the output, go back to step 1).
    4. git commit

@colemanw
Copy link
Contributor Author

colemanw commented Oct 4, 2023

@totten I'd be fine with adding a new command namespace for export. I wasn't comfortable doing that since I don't know my way around the civix internals that well, and wasn't sure this PR would cross the threshold of meriting a new namespace. But sure. (aside, the generate: space feels a bit overused, and some of those commands are very long to type -- I'm pretty tired of typing out civix generate:entity-boilerplate!)

I find it easier to understand export by name than export by ID.

Well... ok but not every entity has a name. And the APIv4::Export action takes id as a param, not name. I don't think it makes sense to convert ExportAction into a BatchAction because it's designed to spit out the code for one file, and it already has its own recursive logic to include multiple linked entities in that file (e.g. OptionGroup + all its OptionValues). But potentially the civix command could be be based on the GetAction and then loop through the results calling ExportAction by id to generate one file per get result. That would work, although we'd need to make +w required so you don't accidentally export every record in the database.

It's pretty tempting to split these out...

Yes, that was tempting, but I settled on not doing that because exporting an Afform actually can result in the creation of one (or more) .mgd.php files for the embedded SavedSearch(es)+SearchDisplay(s). So there are differences but a lot of overlap too, and I thought it might be simpler for the civix user to not need to memorize the differences but just call one command for everything exportable.

If we change the name of the command to civix export then I'm even less inclined to make different commands for afforms and other entities, since the term "export" is generic enough to cover both.

It would be nice to have a "re-export" or "export-all" command.

Sure, although this command is already set to overwrite files instead of aborting if the file exists, since I pictured that use-case of wanting to iterate on your export. So you can already do what you described with the current command by passing the id of the thing you want to re-export.

@colemanw colemanw changed the title Add generate:managed command to export managed entities and afforms Add export command for managed entities and afforms Oct 25, 2023
@colemanw
Copy link
Contributor Author

@totten ok I've cleaned up the commit history and pushed an update which renames the command from civix generate:managed => civix export and extracts a few functions for readability/reusability. Updated your unit test.

I gave this some real-world testing by using the command on https://lab.civicrm.org/extensions/cividiscount/-/merge_requests/284 to update the Afforms generated 10 months ago. Using this command it converted all the .aff.json files to aff.php format. It automatically transformed the contact_summary key into placement (thanks to the compatibility layer added in civicrm/civicrm-core#27755) and it broke out the navigation into its own .mgd.php file following the new convention to omit domain_id and let core handle it.

All-in-all I'm happy with it. What do you say?

For example:

* Install `civi_campaign` and `civicrm_admin_ui`
* Run `cv api4 SavedSearch.get +s id,name -T`. Note the id of `SavedSearch_Administer_Campaigns`
* In `civicrm_admin_ui`, export `SavedSearch_Administer_Campaigns`
* Problem: We now have two modules trying to provide `SavedSearch_Administer_Campaigns` (with the same value of `civicrm_saved_search.name`)

The mistake here is that this search is actually owned by `civi_campaign`.
Allowing `civicrm_admin_ui` to make a copy with the exact same content (but
a different `civicrm_managed` record) ensures a conflict. This situation is
most likely a user-error (e.g. you forgot where the mgd was supposed to
live, or you didn't pay attention to PWD).

`
@totten
Copy link
Owner

totten commented Nov 16, 2023

civibot, test this please

@totten
Copy link
Owner

totten commented Nov 16, 2023

Using this command it converted all the .aff.json files to aff.php format. It automatically transformed the contact_summary key into placement (thanks to the compatibility layer added in [civicrm/civicrm-core#27755](https:

Wow, that's pretty cool!

I like how you can do some edits in the database, re-export, and run git diff. I'm fairly excited by the idea of adding a civix export --all flag (which would query Managed::get and re-export any DB edits).

There is a big caveat -- it only regenerates correctly if it created the original file. When I look through universe, I see a huge number of *.mgd.php files that aren't gonna regen correctly. Relatedly, the file-layout is (from a civix.git POV) an outlier -- it names mgd's differently from all the other generators.

  • Quick data: in my copy of universe, I see ~280 mgds in civix-style layout; ~180 mgds in searchkit-style layout; and ~30 mgds with a minimalist/top-level file. Within those buckets, there can still be variations -- e.g. a lot of searchkit-inspired layouts use slightly different names for the folders or the files or the entities.
  • We could explore the paths to unifying file-layouts (i.e. switch SearchKit mgd's to look more like civix mgd's; or switch civix mgd's to look more like SearchKit mgd's; in either case, ultimately push downstream repos to reorganize files)... but that seems quite disruptive...
  • This sounds better: update civix export to recognize/preserve the names of existing mgd's. The upshot:
    • It's compatible with widest range of existing extensions/entities.
    • It relieves the pressure to get the layout "just right". We can iterate on file-layout without affecting the correctness of the re-export workflow.
    • Downstream developers retain ultimate authority over their file trees.
    • Eventually, this could provide a way to convert APIv3-based mgds to APIv4-based mgds.

Of course, all of that speaks to follow-up work. For the moment, this PR does the work it was aiming to, and I know your anxious to get the functionality out there. I've just added a couple more sanity-checks/warnings. We can obviously continue to discuss/iterate in other PRs.

@totten totten merged commit 84dd21b into totten:master Nov 16, 2023
@colemanw colemanw deleted the export branch November 16, 2023 12:20
@colemanw
Copy link
Contributor Author

There is a big caveat -- it only regenerates correctly if it created the original file.

@totten can you clarify what you mean by "file-layout"? Are you referring to filename? Or layout of code within the file?

If the former, FYI this PR simply uses the name returned by APIv4::export action. That's the only real "standard" there is, everything else is just "make it up". Traditionally, filenames do not matter with .mgd.php but it's true they would matter if you wanna consistently regenerate the same file.
As this civix export command becomes the go-to tool for generating such files we'll start to see more uniformity with .mgd.php filenames throughout universe.

If the latter, I'm not quite sure what you mean, but I would expect there are some code-style inconsistencies between autogenerated php files and hand-written ones. These files do pass civilint though.

@totten
Copy link
Owner

totten commented Nov 20, 2023

(@colemanw) can you clarify what you mean by "file-layout"? Are you referring to filename?

I see (basically) three file naming patterns. I expected two of these (based on things we've published), and the third was originally a surprise to me (but makes sense in retrospect). The patterns can be roughly described as:

File Layout Spiritual Origin Description Often Used With...
1 Adjacent Files Civix Generators Create some PHP logic. Create an adjacent *.mgd.php file next to it. Jobs, Reports, Payment Processors, Custom Searches
2 Single Folder SearchKit Exporters Create a top level folder (managed). Add lots of managed entities here. Saved Searches, Search Displays, Option Values
3 Single File (Unassessed) Create a top-level file (myextension.mgd.php). Add various entities here. (Unassessed)

Just for illustration, here are few random examples:

## Adjacent files
./coop.symbiotic.paysafe/CRM/Core/Payment/Paysafe.mgd.php
./com.iatspayments.civicrm/CRM/Core/Payment/Iats.mgd.php
./de.systopia.segmentation/CRM/Segmentation/Form/Search/SegmentSearch.mgd.php
./areas/api/v3/Contact/UpdateAreas.mgd.php
./archivemailing/api/v3/Job/Archivemailing.mgd.php

## Single folder
./certifications/managed/SavedSearch_Certifications.mgd.php
./chatbot/managed/activityStatuses.mgd.php
./de.systopia.eck/managed/OptionValue_cg_extends_objects.mgd.php
./uk.co.vedaconsulting.mosaico/managed/Mosaico_Template_Listing.mgd.php
./monolog/Managed/SavedSearch.mgd.php

## Single file
./btcpay/btcpay.mgd.php
./cashpp/cashpp.mgd.php
./ukgiftaid/civigiftaid.mgd.php
./firewall/firewall.mgd.php
./uk.co.vedaconsulting.mautic/mautic.mgd.php

(Aside: Each has some adhoc variations -- like managed/ vs Managed/ vs Metadata/. But IMHO those variations don't change the big picture.)

These layouts have strengths and weaknesses. None is perfect for everything.

  • Adjacent Files: Makes it easy to cross-reference between code+registration. (Ex: You have a Job and ask: What are the parameters are typically passed in? Just look at the adjacent mgd.) But sometimes you don't have any PHP file to be adjacent to.
  • Single Folder: Easily accommodates configuration-elements/data-only entities. (Ex: A "SavedSearch" is a configuration-element. You don't have PHP classes/functions to go alongside each.) But the one folder gets messy if you have many items.
  • Single File: It's flat and simple. You don't have to think about the directory tree. But the file gets messy if you have many items, and (also) no one has written tooling or done advocacy for it.

Heretofore, civix.git has been consistent/uniform -- it has always used Adjacent Files layout. (Historically, the original purpose of *.mgd.php was to enable Adjacent Files...)

This PR follows the Single Folder layout -- the PR makescivix.git inconsistent. (Different generators produce different layouts.) Of course, reading between the lines, the major intent is to support SavedSearches and SearchDisplays -- i.e. things which haven't been generated by civix before. The inconsistency may seem superficial at first. (*Generating a SavedSearch with SingleFolder layout won't break a Job that uses AdjacentFile layout.)

But civix export is not just like any generate:foo command. It allows you to re-export. It enables a new workflow and it purports to work with any entity. That's all very cool. The problem is... if you actually use the new workflow on any entity generated by civix... then it won't work. With this PR, the different parts of civix aren't working with each other -- specifically because this command follows a different file layout. (Also, because we need more entities in APIv4... but that's a separate ball of wax...)

@colemanw
Copy link
Contributor Author

Oh wow, I didn't realize civix already exports .mgd.php files. 🤯
I just looked & see 2 generators that do so:

  • AddReportCommand
  • AddApiCommand

APIv3 and CiviReport are both on their way out IMO. We could just leave them alone and let them die a natural death.

@mattwire
Copy link
Contributor

Just a note that I've switched to always putting mgd.php files in managed/ as I find it much cleaner/easier to find rather than putting them in various places (eg. CRM/Core/Payment). I usually update extensions and move them to the managed/ folder when making changes.
I think @eileenmcnaughton may have started the managed/ trend and I like it.

@eileenmcnaughton
Copy link
Contributor

@mattwire I have a nasty feeling I mix & match managed & Managed though..

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.

4 participants