Move hook_civicrm_translateFields from message_admin to core

[eileenmcnaughton]

Overview

Move hook_civicrm_translateFields from message_admin to core

#23844

Before

The fact that message template fields are translatable is in the message admin UI extension. However, rendering translated is core functionality (well once the rest of #23844 is merged) it will be - so the right place to define it is in core where the functionality is

After

Moved

Technical Details

This is the part of #23844 that is causing test fails so I'm separating it out - also, it should be an easy merge when tests pass

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@totten do you think this test can work in this context - or should we skip it?

[totten]

However, rendering translated is core functionality (well once the rest of
#23844 is merged) it will be - so the right place to define it is in core
where the functionality is

I don't feel too strongly about where hook_civicrm_translateFields bit lives, but "the right place" is a bit ambiguous. My understanding is that the ability to enable/disable translation (at runtime, per entity/field, per configuration/hook/etc) was one major goal for the Translation approached (compared to existing multi-lingual, which is more static). ie Multilingual has a singular/right place (xml/schema), but Translation does not (hence a hook).

What that means is that MessageTemplate::sendTemplate() (et al) should be defined in a way that works in either configuration (ie the same code works, regardless of whether the local policy is to enable or disable translation). From that POV, the implementation of sendTemplate() (et al) should not depend on the placement of hook_civicrm_translateFields. (It will, however, depend on the support for $preferredLanguage.)

But there are a couple other POVs, ie:

• Unit test POV: When writing a test-suite for sendTemplate(), we should have (should be able to have) test-cases for each configuration (testing sendTemplate with and without translation configured).
• Install/setup POV: For a particular deployment, does the site-builder prefer to have translation enabled or disabled? (Do they opt-in or opt-out? How?)

I think that the placement in message_admin lined up nicely with the "Install/setup POV", but "CRM_Core_BAO_MessageTemplate" is also OK from this POV.

From discussion, it sounded like the the "Unit test POV" is where this became important -- ie in terms of writing tests, you'd rather have the presumption that all/most deployments have translation enabled. (Presumably, for test-cases with translation disabled, then you'd have to do some extra declarations in the test to opt-out.)

[totten]

tldr: I'm OK with moving it (because I think the placement shouldn't matter too much). Just want to make sure that we're keeping that posture (it's still going to be loosely coupled in implementation, even if the default policy is shifted around a bit).

[eileenmcnaughton]

Yeah I think the message_admin extension always felt actively wrong to me (as it should be just a UI) - although I could argue a new extension for translation config makes sense rather than core - hence I didn't think it made sense to bend over backwards to keep it there for tests to pass

[seamuslee001]

Jenkins re test this please