Civix could (or should?) offer in-line-replacements for dropped deprecated functions

[bjendres]

CiviCRM core dropped the (long deprecated) CRM_Core_OptionGroup::getValue with version 5.60, which caused a bit of havoc for us (see e.g. here). Sure, we could've replaced this a long time ago, but then again there was no deadline... so it never ended up at the top of anyone's list.

What we did, is to add a drop-in replacement functions in the extension's code, so the act of making the extension compatible is really easy, see here. We put the code into our CustomDataHelper, which is essentially a poor man's civix mixin (plus some other stuff).

I think, a far better place for the implementation of such drop-in function replacements would be civix.

It could even apply the replacement of deprecated functions within the civix upgrade process.

@totten what do you think?

[eileenmcnaughton]

There are literally thousands of functions in CiviCRM and any one of them could be called from an extension, so I don't know how realistic it is to do something like that for every function we deprecate out of core.

I do encourage people to copy core functions to extensions rather than to call core functions, other than the api & a few specific classes that are clearly supported. What Tim did suggest was a tool to find deprecated functions (much like our IDEs provide).

Note that the recommended syntax for these functions is to use the PseudoConstant class - ie CRM_Core_Pseudoconstant::getKey() see https://docs.civicrm.org/dev/en/latest/framework/pseudoconstant/

Having said all that - the CustomDataHelper seems to just be doing the same thing as the core Managed system? Maybe it would be better to just use the Managed system - which now has the export action in the api explorer that creates the php file to bang into the extension

[bjendres]

There are literally thousands of functions in CiviCRM and any one of them could be called from an extension, so I don't know how realistic it is to do something like that for every function we deprecate out of core.

I see your point. On the other hand, we maintain over a hundred extensions, so we need a more or less standardised approach - one where we have a set of in-line replacements for each deprecated function in a common component, so the actual update is a search&replace kind of affair - otherwise this is not manageable for us.

IMHO, the location of the replacement code could be in:

1. our CustomDataHelper which we wanted to phase out anyway (seeing that half of the functionality can now be replaced with managed entitites). Not ideal, but a quick fix.
2. a separate "legacy" extension that we can list as a dependency, providing an in-line replacement for each dropped deprecated function. That works, but it would have to be around for a long time.
3. civix. This would have the advantage that it already supports sophisticated code alteration, and could probably do the changes with a "civix upgrade".

I can understand that you're uneasy with 3., and if you say that it's not happening, we'll probably focus on 2.

[jensschuppe]

Thanks @bjendres! I'd vote for some kind of implementation in Civix (identification or doing recommended replacements or doing legacy-style replacements) as long as CiviCRM Core will go on removing deprecated functionality in minor releases, which is forcing extension developers to adapt to deprecations with at least the next security update of CiviCRM Core after removal of code.

[eileenmcnaughton]

Hmm - it's worth noting that this will have been putting out deprecated notices for a long time if you had your php allowing these to show - we have over time removed a considerable number of internal core functions - but we do commit to not changing those functions that we support for external use - ie in this case the supported alternatve has been around since 2014 and the getValue function was annotated as deprecated in 4.7 in 2017

So iI'm sure we would be OK with a PR to add a mixin for getValue and the other deprecated functions removed (I don't know that you actually added a helper for alll the functions removed at that point, or the other functions removed at various other points) - but if feels like it might be a better use of time to do a bit of an audit of what deprecated core functions you are calling & also what core functions you are calling that are not API functions or hooks and copy those into your extension.

It's also worth noting that unit tests fail when they hit these deprecations so if you had been running CI over these extensions you would have had to fix them years ago - which also feels like a better use of time.

[MegaphoneJon]

I think we're in agreement about the problem but not the solution.

Deprecation notices are buried in the Civi logs which has a poor signal-to-noise ratio. Many deprecation notices (at least historically) are triggered by the core code itself. It's difficult to find the ones that need action taken.

I suspect that if there was better visibility on use of deprecated functions, that would satisfy everyone's needs.

CiviCRM has always been hard to statically analyze - its autoloader is quirky and the CMS integration makes it worse. I haven't looked at Bradley's PHPStan for Civi work but I suspect if it was easy for folks to generate reports of deprecated functions in extensions this need would be far less urgent.

[eileenmcnaughton]

@MegaphoneJon don't the deprecation notices show up for you when you navigate the site (on dev instances) - because as long as your error reporting permits (which is should on dev sites) they should (they show up red in the UI for me although I normally pick them up through unit tests)

[MegaphoneJon]

@eileenmcnaughton Sometimes, but a) Smarty notices make it hard to spot deprecation notices, b) more often than not, deprecation notices are happening during non-interactive code execution - CiviMail, CiviRules, etc.

[eileenmcnaughton]

Ok - it would be good if civi-generated deprecation notices bubbled up to system check errors - deprecations are pretty easy to find in an IDE - but people have to be looking.

[totten]

@bjendres Good topic. The thread has gone a few ways, so I'll just post some thoughts in no particular order:

1. I definitely see the similarities between Custom-Data-Helper, the civix templates, the mixins, and specifically mgd-php. If you're interested, we could talk about how to convert Custom-Data-Helper to a mixin. The upshot: when/if there's a future change (like swapping-out CRM_Core_OptionGroup::getValue()), then subsequent upgrades would be easier. (Could update a mixin without re-publishing every extension.)

2. The scale and cadence is fairly different for civix upgrades (~10, covering changes from several years) and for civicrm-core deprecations (~350 circa 5.63.alpha; that includes ~100 from just 2023-YTD).

3. There are important differences between showing warnings and automatic upgrades/semi-automatic upgrades.

4. Automatic Upgrades / Semi-Automatic Upgrades / Code Manipulation

   • Each automatic upgrade has its own cost-benefit consideration -- weighing the #impacted extensions, the complexity of the change, and the labor for writing the update.

   • Speaking generally, the cost-benefit of automating all transitions for civicrm-core methods would be... awful. However, for specific transitions, it could very useful.

   • Hypothetically... say you have 20 public extensions and 20 private extensions where CRM_Core_OptionGroup::getValue() needs to change. It would probably be worthwhile to add a one-time upgrade step for that. (I don't personally have sufficient data to persuade myself to work on that. But I think we should be open to that kind of thing.)

   • There are different techniques to facilitate meta-programming (like regex, php-generator, and rectorphp). IMHO, some would offer better quality/efficacy. (regex would struggle with use CRM_Core_OptionGroup as OG; rectorphp is probably better.) But I don't think any of these would make the cost-equation better.

5. Warnings / Reports

   • I tend to agree with @eileenmcnaughton that the easiest way to get a report with deprecation-warnings is to open the code in PHPStorm.

   • PHPStorm is awesome, and I use it every day, but it does have issues. It's closed-source. A lot of the warnings are "meh" (IMHO). It's awkward to share settings. It's hard to plugin to other workflows. To get an up-to-date assessment, you have to make a local-build with your target-extension and the target-version of CiviCRM.

   • IMHO, in an ideal world:

     • Developer can get a report about deprecations affecting their extension and civicrm-core@latest (regardless of what version they happen to use for local-dev/client-staging).
     • Sysadmin can get a report about deprecations affecting all extensions on their system (for the live version of civicrm-core; for a planned/pending upgrade; and for latest).
     • Reports would include notes about the things that are deprecated and also things that have been deleted.
     • Reports would be included in IDE (without needing 20 configuration steps).
     • Reports could be easily generated by CI.
     • (End of wishlist)