Closed colemanw closed 10 months ago
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!
@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:
+w
parser from cv
. Or probe the target entity, trying to match by ID or name. Or have multiple options)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
civix re-export
(or civix export --all
or somesuch)git diff
(If you don't like the output, go back to step 1).git commit
@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.
@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 https://github.com/civicrm/civicrm-core/pull/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?
civibot, test this please
Using this command it converted all the
.aff.json
files toaff.php
format. It automatically transformed thecontact_summary
key intoplacement
(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.
civix export
to recognize/preserve the names of existing mgd's. The upshot:
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.
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.
(@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.
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.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 SavedSearch
es and SearchDisplay
s -- 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...)
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.
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.
@mattwire I have a nasty feeling I mix & match managed & Managed though..
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.php
s for the search displays). The resulting file includesE::ts()
around translatable strings.For a full demonstration, try exporting an existing afform, e.g.:
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.