Simplify hook_civicrm_config, remove extension from include_path

[colemanw]

Since #265 the include_path addition has not been needed except for odd edge-cases where the extension has stuff that's not covered by the autoloader. I'm honestly not sure what those things might be (maybe APIv3 files?), but best-practice is to use the autoloader and not mess with include_path.

[colemanw]

@totten this change doesn't require a civix upgrade step since .civix.php is regenerated whenever civix does anything. But perhaps it would be wise to add an empty upgrade step that just spits out a warning that if anything in the extension relies on include_path they ought to add it using hook_civicrm_config() since the delegated hook no longer does it for them. Or it could offer to add it for them.

[totten]

I definitely like the idea of removing include_path from the default template. But in terms of migration process... I think it would be worthwhile to add a mixin (eg [email protected] or similar) - along with an upgrade-step which conditionally activates [email protected].

Why? Well...

1. I spent an hour or two grepping to figure how many services might be relying on include_path, eg

   https://gist.github.com/totten/c939a3fb4b8510d3f566015d8f9b01a2 (core search) https://gist.github.com/totten/1e7bf55e669cda6684bd69af25889da0 (contrib search)

   To my eye, this gives use-cases like:

   • Old-style CRM_* class-loading (matches almost all pre-existing ext's; but you can now use <classloader> instead)
   • APIv3 (for ext's with any entity or action; matches 125-250 ext's in my copy of universe)
   • APIv4 (for ext's with DAO-based entities; 30-40 ext's; but it's only an issue when deploying on civicrm-core@5.37-5.42)
   • Special/bespoke file-scans which are...
     • Performed by core (for CRM/Dedupe/BAO/QueryBuilder/*, CRM/*/WorkflowMessage/*, CRM/Utils/Address/*, CRM/Utils/Geocode/*, templates/CRM/Contact/Form/Task/Map/*)
     • Performed by contrib (org.civicrm.tutorial searches for crm-tutorials/*.js; nz.co.fuzion.civitoken/civitoken.php searches for tokens/*.inc)
     • (I haven't looked carefully, but I suspect each of these has only a handful of implementations.)
   • Class overrides (no count; annoying to figure out; won't work with <classloader>)
   • (In theory, there may also be some kind of special PEAR add-ins -- like a custom "DB_" or "Mail_" class? But this seems quite obscure/unlikely.)

2. The thing is that these services are really all over the place, eg

   • Some are in core... others in contrib...
   • Some are widely used... or used a few times... or never used...
   • Some have newer alternatives already... and some don't...
   • Some have long-standing support... some are deprecated... and some were short experiments... and some have always been discouraged (but people did them anyway)
   • Most are PHP files... but some are JS or TPL...

3. To facilitate phase out of include_path, we could...

   • Do nothing (or just do a informational alert) -- but I feel like that list is long enough and complicated enough that it would hurt civix's usability/reputation.

   • Add a generic mixin (include-path@1). This mixin would be default-off, but civix upgrade could enable it. Something like this...

     // upgrades/22.11.0.php
     $folders = ['api/v3', 'CRM/Dedupe/BAO/QueryBuilder', 'CRM/*/WorkflowMessage', 'CRM/Utils/Address', 'CRM/Utils/Geocode', 'templates/CRM/Contact/Form/Task/Map`, `crm-tutorials`, `tokens`, 'Civi/Api4',];
     $existingFolders = array_filter($folders, thisExtensionHasThisFolder);
     $io->writeln("Historically, civix relied on the 'include_path' to load 'CRM' classes. The <classloader> directive now provides better support for this. For many extensions, the 'include_path' is now redundant.");
     $io->writeln("However, the 'include_path' may be required for other reasons.  For example, autoloading these folders may rely on `include_path`: $existingFolders");
     if ($io->confirm("Should we enable or disable continued support for 'include_path'?")) {
        // enable or disable the 'include-path@1` mixin
     }
     

     The nice thing approach is that we can remove include_path for most extensions -- while providing backward/forward continuity -- and it only requires one upgrade-step.

   • Assess each service 1-by-1. For example, maybe geocode and dedupe-query-builder should switch to AutoService, while api3 and tutorial should have their own mixins,

     This might arguably give a better end-state. (More precise metadata? More stylistic consistency? More options for refactoring?) However, it seems like a lot more work -- because you have to update ~8 loaders, and think about the compatibility questions for each, and then document-or-automate the migrations for each. But if anyone wants to flag this as higher-value approach, then we could go down that path.

If there's no one who wants to encourage the 1-by-1... then I'd be pretty inclined toward adding a generic include-path@1 mixin...

[colemanw]

@totten I like your mixin idea. Just to distinguish between our 2 suggestions:

• I suggested that civix could offer to write set_include_path(...) directly into the module.php file's implementation of hook_civicrm_config(), basically un-delegating it from _civix_civicrm_config().
• You suggested doing the same thing but in a mixin rather than a hook, which is definitely tidier.

From a code-organization POV a mixin does sound nicer and I'm generally +1 for that idea, however are we concerned about loading this during bootstrap? For reasons I still don't understand, it's been suggested (see #272) that hook functions are available earlier in the bootstrap process than mixins. Is that really the case and if so would that cause problems for anything that needs to be included early?