Conversation
In those cases my typical recommendation would be to declare the dependency. Unless there's a compelling reason not to, that's orders of magnitude simpler than writing and maintaining your own class scanner. |
So I originally drafted prose trying to describe both options. I dropped the comments about declaring the dependency because the prose was getting long -- and because the custom scanner looked like the simpler recommendation. 😄 These things are subjective. Here's the angle I was looking at:
Of course, to your point -- if someone defines any kind of custom scanning rules, then they could get in trouble later... e.g. if forget about this and starting adding code outside of their scanned-folder. Perhaps the missing element here... data. I tried to get some real data.. CiviRules: Consumers
Legacy Custom Search: Consumers
Alas, those numbers aren't very clear -- we do see multiple examples of both faulty omissions and valid omissions, but there's no statistic on them. My gut read of the list is that faulty omissions are probably more common than valid omissions. (But not enough that we can turn on blinders.) So maybe the current draft is too simplistic (i.e. being quiet about the possibility of faulty omissions). To get more bite-sized prompts, I'm thinking to split out two distinct upgrade-steps:
|
That sounds like a good plan! |
|
New screenshots - these are for an extension with an undeclared dependency on CiviRules:
|
|
@totten this looks good! Nitpick: the first dependency check is a bit verbose for a noop. I found myself reading "[INFO] Found no evidence..." twice to assure myself that "no evidence" is actually a good thing. Had it been me, I probably would have suppressed the entire message if no problems are found. But I know you like giving feedback. Maybe it could at least be shortened to something unambiguously ✅ like "no problems found" |
|
@colemanw Yeah, I agree that screen is a bit ambiguous. Latest push should be a little quieter and little less ambiguous. |
|
Looks ready to merge @totten |
2 participants
Overview
The class-scanner allows you to create new PHP files with services and listeners -- without needing to hand-edit or re-generate the main
*.phpfiles. This encourages modular development, and it makes it easier to copy/borrow/move snippets of code. The PR encourages adoption with main two revisions:civix generate:module: On CiviCRM v5.51+, enablescan-classes@1by default for new extensions.civix upgrade: Prompt the developer to enable the scanner. Include the known counter-indicators (i.e. exceptions where you may not want it) and alternatives (e.g. custom scanner).This is generally an outgrowth of work on civicrm/civicrm-core#33371, which will eventually require class-scanning for
Civi/Api4. It aligns with @ufundo's argument (which I mostly agree with) that class-scanning is a good general practice. It specifically expands on @colemanw's #412 (and borrows some ideas from that discussion), but this is a bit more comprehensive and more nuanced. In my local system, phpunit is happier. (We'll see if Jenkins agrees.)Technical Details
There's not much to see in the
civix generate:module. Most of the action comes incivix upgrade.Here is a preview of
civix upgradeonmosaico, which should be fairly typical:Or here's an example with a more difficult extension (where its
<compatibility>is older and it has legacy custom-searches):Based on anecdotal reading of
universe, I believe there are some common cases where extensions have undeclared dependencies. I've added heuristics to detect two of them:CRM_Civirules*but don't declare a full dependency onorg.civicoop.civirules.CRM_Contact_Form_Search_Custom_Basebut don't declare a full dependency onlegacycustomsearches.If it finds these clues, then it switches its default recommendation (
scan-classes@1=>hook_scanClasses). It's OK to add more heuristics.Of course, it would be even better if we find a design that supports a standard scanner (with little/no configuration) while still supporting these. I look forward to talking more with @jaapjansma @ufundo etal in a couple weeks about that.