Development issueshttps://lab.civicrm.org/groups/dev/-/issues2024-01-14T14:55:57Zhttps://lab.civicrm.org/dev/core/-/issues/3613System Workflow Messages - Improve localization experience2024-01-14T14:55:57ZtottenSystem Workflow Messages - Improve localization experience## Background
Several CiviCRM components - such as CiviContribute, CiviCase, and CiviEvent - send automated email notifications. Many organizations find it useful to customize/tune these emails, and the emails may be administered in the...## Background
Several CiviCRM components - such as CiviContribute, CiviCase, and CiviEvent - send automated email notifications. Many organizations find it useful to customize/tune these emails, and the emails may be administered in the web UI ("Administer: Message Templates: System Workflow Messages").
In principle, this is a powerful tool in which an administrator may use Smarty templating to produce highly tuned messages -- and it requires only the ability to understand template notations (e.g. `{$contact.first_name}` or `{$event.start_date}`). However, in practice, this is often difficult -- real-world templates are long and complex, and they may be used with data of varying quality/character, and the workflow to test a template can be tedious.
## Goals
This epic (*overarching issue*) aims to improve the experience of maintaining system workflow templates. We are specifically funded to work on it from the following perspective: You have an organization which maintains multiple templates for use in multiple countries/locales -- periodically, as campaigns/operations are revised, you may need to engage with a few people to update+retest a few templates. We have three main themes to improve upon:
* __Drafting/Workflow__: Currently, Civi has exactly one variant/revision of a template, and it is always live. There is no way to develop a draft (new revisions) or to keep a record of past revisions.
* __~~Translation~~ Localization__: For any given message, you may need to compose different versions for different locales.
* __Previews/Samples__: Many templates are sensitive to fine-grained details. This is true in multiple ways, e.g. (1) the template adjusts based on *available data* ("Do we have a mailing address for this person? Do we have a full-name, or just an email? Is this one-off contribution or a recurring contribution?"), and (2) the template includes *precise codes* that must be well-formed (e.g. matching tags, matching quotes, using an email-friendly dialect of HTML/CSS).
## Related work
* https://lab.civicrm.org/extensions/msgtpltools
* https://lab.civicrm.org/community/feature-request/-/issues/26
* https://github.com/eileenmcnaughton/civi-data-translate
* https://lab.civicrm.org/extensions/l10nx
## Subtopics / Brainstorming
(Some of the descriptions here are terse - but they may get more explanation in the wireframe.)
* __Sample data presentation__: While composing a message, how do you show a preview based on sample data? Some ideas:
* __Edit-Preview Split-Panes__
* __Pro__: very amenable to live-update
* __Con__: needs wide screen (prob dialog). agreeable with O(10) sample records - but disagreeable with O(1000) records
* __Edit-Preview Accordion__
* __Pro__: fairly amenable to live-update. full width.
* __Con__: agreeable with O(10) sample records - but disagreeable with O(1000) records
* __Explore-Preview Panes in dialog__
* __Pro__: agreeable with O(1000) records
* __Con__: requires more navigating.
* __Comment__: live-update might be possible with separate popup - but more difficult
* __Comment__: might mitigate navigation work with a hotkey
* __Sample data source__: When viewing a preview based on some example record(s), where does the example come from?
* __Basic/prepackaged__: Define some JSON files, which are linked to the workflows.
* __Comment__: Requires metadata to say, "workflows $a+$b can use sample data $c+$d"
* __Pro__: Some templates have very nuanced purposes and may have special vars. This allows you to mock data that's attuned to edge-cases, and you can use these samples in any environment (dev-site/public-demo-site/live-site). No mixing-up real records and sample records. Can simulate complex data (e.g. "tpl for use-case with 2 contribution records")
* __Con__: More upfront work in making sample data. User cannot test against real data.
* __Comment__: Should have unit-test to ensure sample-data-structure stays in sync with real-data-structure
* __Data search__: Search for a live record (eg `Contribution` or `Membership`) to use as a sample.
* __Comment__: Requires metadata to say, "workflows $a+$b need data from entity $c"
* __Pro__: You can use real data, which can be subjectively attuned.
* __Con__: You have to find real data that's *useful*. May be awkward if you're trying to test edge-cases, unless you're willing to add mock/synthetic records to the DB. All mail-merge data must be selectable from one record (eg "Contribution #123"; eg no other inputs besides DB content).
* __Variants__: The UI for this can take on a few variations, eg
* In "Edit Msg Tpl", show a searchable tree. Filter by contact name and select the target record.
* In "Edit Msg Tpl", ask the user to enter a numeric ID.
* In "View Contact", add links/actions for generating previews.
* __Flagged records__: As an adminsitrator, find real records and save them in a list (e.g. "Record #$x is a sample for use in workflows $a, $b")
* __Comment__: Requires somewhere to store the list (e.g. custom-field or new-field or new-table)
* __Pro__: You can use real data, which can be subjectively attuned. The list of samples is curated/abbreviated.
* __Con__: The individual composing the template may not know how to flag/unflag sample records. Requires configuration (and extant samples) before you can use email preview.
* __Grain of ~~translation~~ localization__
* __Record level__: For each `(locale,workflow)`, create another `civicrm_msg_template` record.
* __Field level__: Each `(workflow)` only has one `civicrm_msg_template` record. Attached to that record are multiple translations (ex: `field=subject,locale=fr_FR,text="Bonjour"`).
* __String level__: Each `(workflow)` only has one `civicrm_msg_template` record, and that record has one `body_text`. However, the `body_text` uses translatable strings (ex: 'body_text={cts id=greeting}Greetings,{/cts}`)
* Trade-off
| Record-level | Field-level | String-level |
| -- | -- | -- |
| Each translation goes through a separate, well-delineated drafting workflow. | Translation workflow is batched (per template). All translations must be ready to activate together. | Orthogonal workflows for templates vs strings. |
| Message structure (paras/sections) is localized. | Message structure (paras/sections) is localized. | Message structure is uniform across locales. (Realistic? What abouts `<table>` LTR/RTL? |
| Cannot re-use translated snippets. | Cannot re-use translated snippets. | Re-use translated snippets. |
## Wireframes
There are a few different ways to approach this. To get a sense for the usability and work, we can post a few alternative wireframes. (*This section may be updated as more are added.*)
* __(0) Status quo__: [_0__Sys_Wf_Msgs](/uploads/ad96003d71133d298f95ce79902a7583/_0__Sys_Wf_Msgs.png), [_0__Edit_Msg_Tpl](/uploads/81bc234651182b65993bc28c8052b68b/_0__Edit_Msg_Tpl.png)
* __(1) Record-level translations w/previews and drafting workflow__: [_1Rec__Sys_Wf_Msgs](/uploads/532b842406c17c31be5ae9195ceb6aa8/_1Rec__Sys_Wf_Msgs.png), [_1Rec__Edit_Msg_Tpl](/uploads/13a1f717e829e3de480750c4893969a7/_1Rec__Edit_Msg_Tpl.png) (*Note: The blue-arcs indicate a couple variations on how to include the preview/sample functionality. This tracks close to status-quo, and incremental changes are highlighted with yellow-dots.*)
* __(2) Field-level translations__: [_2Fld__Sys_Wf_Msgs](/uploads/7e1f46e722d8afc706a92c56e295c55e/_2Fld__Sys_Wf_Msgs.png), [_2Fld__Edit_Msg_Tpl](/uploads/4bc932b7bb3f304853b0c107b98d41b5/_2Fld__Edit_Msg_Tpl.png) (*Note: This tracks close to status-quo, and incremental changes are highlighted with yellow-dots.*)
* __(3) String-level translations__: [_3Subfld__Sys_Wf_Msgs](/uploads/f52ffbb764917954790fbbd3ed807acf/_3Subfld__Sys_Wf_Msgs.png), [_3Subfld__Edit_Msg_Tpl](/uploads/b20bc83d302d55a485d975aff6ca70e1/_3Subfld__Edit_Msg_Tpl.png) (*Note: This tracks close to status-quo, and incremental changes are highlighted with yellow-dots.*)
* __(4) Record-level translations w/browse-edit tree + accordion-preview__: [_4Tree__Browse-Edit](/uploads/8cc47238a9d597f80950c1d98c0ea43a/_4Tree__Browse-Edit.png)
* __(5) Field-level translations w/browse-edit tree + accordion-preview__ [_5Tree__Browse-Edit](/uploads/aab231199d0b4217a2f44f9ad45aecb1/_5Tree__Browse-Edit.png)
* __(6) Field-level translations w/matrix, preview-dialog, and drafting workflow__: [_6Mat__Sys_Wf_Msgs](/uploads/3a32390f36f50d8e158e1155dc586969/_6Mat__Sys_Wf_Msgs.png), [_6Mat__Edit_Msg_Tpl](/uploads/353f97846fd9d2b1e037ec92e60101fc/_6Mat__Edit_Msg_Tpl.png) (*Note: This assumes that there is a separate string-storage service like [civi-data-translate](https://github.com/eileenmcnaughton/civi-data-translate), and it assumes that the `String` record has some workflow property like like `is_draft` or `revision`. So each string might have the properties `entity`,`entity_id`,`field`,`locale`,`text`,`is_draft`.*)
* __(7) As with 6, but with various refinements__: [_7Mat__Sys_Wf_Msgs](/uploads/73d37acab4307054b4fe4f3291ae7dcd/_7Mat__Sys_Wf_Msgs.png),
[_7Mat__Edit_Msg_Tpl](/uploads/4807652bd20bfb5bf0912bf24f05d444/_7Mat__Edit_Msg_Tpl.png)https://lab.civicrm.org/dev/core/-/issues/4911Add params to actions when creating a WordPress User2024-01-12T17:45:30ZhaystackAdd params to actions when creating a WordPress UserOverview
----------------------------------------
The actions in the `CRM_Utils_System_WordPress::createUser()` method would be more helpful if they provided params when they fire. They were originally added to allow other code to know m...Overview
----------------------------------------
The actions in the `CRM_Utils_System_WordPress::createUser()` method would be more helpful if they provided params when they fire. They were originally added to allow other code to know merely *that* CiviCRM was creating a User (and to prevent recursion) but it would also be useful to know the source Contact and the resulting User ID in some cases.
Example use-case
----------------------------------------
A plugin that acts on a WordPress User when a Contact is added to a Group listens for `GroupContact.create` on the `civicrm_post` hook. Works just fine when using the CiviCRM back-end and the Contact and User both exist.
Current behaviour
----------------------------------------
The plugin will fail to act when a new Contact is added to a Group via a Contribution Page with a Profile that is configured to create a WordPress User. This is because the Contact is added to the Group _before_ the WordPress User is created.
Proposed behaviour
----------------------------------------
Although not an ideal solution to the problem, adding params to the actions in the `CRM_Utils_System_WordPress::createUser()` method would allow a plugin like the above to more easily identify the source Contact and the new User. The GroupContacts can then be checked and acted upon once the User has been created.
Comments
----------------------------------------
PR to follow.haystackhaystackhttps://lab.civicrm.org/dev/core/-/issues/4048Fatal error when changing membership type, on membership with 0 contributions2024-01-12T17:29:15ZBradley TaylorFatal error when changing membership type, on membership with 0 contributions## Steps to reproduce
1. Create a membership without a contribution.
2. Edit the membership you just created, change the membership type, and save (again without a contribution)
## Expected result
The membership should correctly save ...## Steps to reproduce
1. Create a membership without a contribution.
2. Edit the membership you just created, change the membership type, and save (again without a contribution)
## Expected result
The membership should correctly save both times.
## Expected result
The second save does not save correctly/ causes a 500 server error.
The error message is:
```
Fatal error: Uncaught Error: max(): Argument #1 ($value) must contain at least one element
in /app/web/app/plugins/civicrm/civicrm/CRM/Member/Form/Membership.php on line 1547
Call stack:
1. max()
app/plugins/civicrm/civicrm/CRM/Member/Form/Membership.php:1547
2. CRM_Member_Form_Membership::setStatusMessage()
app/plugins/civicrm/civicrm/CRM/Member/Form/Membership.php:1380
3. CRM_Member_Form_Membership::submit()
app/plugins/civicrm/civicrm/CRM/Member/Form/Membership.php:881
4. CRM_Member_Form_Membership::postProcess()
app/plugins/civicrm/civicrm/CRM/Core/Form.php:573
...
```
The membership does appear to actually be saved correctly upon refreshing the page.
## Environment information
PHP 8.1
I cannot reproduce this on dmaster. I think this is because calling `max(array_keys([]))` on PHP 7.4 merely causes a warning, whereas on PHP 8.x it causes an `Uncaught ValueError`.https://lab.civicrm.org/dev/core/-/issues/3958PHP 8.2 deprecations2024-01-11T18:55:35ZseamusleePHP 8.2 deprecationsThis is to be a catch all issue for working on PHP8.2 issues
@bradleyt had already created the following issues for some specific PHP8.2 issues
* [Dynamic Properties are Deprecated](https://lab.civicrm.org/dev/core/-/issues/3833)
* [De...This is to be a catch all issue for working on PHP8.2 issues
@bradleyt had already created the following issues for some specific PHP8.2 issues
* [Dynamic Properties are Deprecated](https://lab.civicrm.org/dev/core/-/issues/3833)
* [Deprecated ${} string interpolation](https://lab.civicrm.org/dev/core/-/issues/3831)
* [utf8_encode and utf8_decode functions deprecated](https://lab.civicrm.org/dev/core/-/issues/3832)
There are probably others out there but adding this to help us navigate to the various sub issueshttps://lab.civicrm.org/dev/core/-/issues/3832PHP 8.2 utf8_encode and utf8_decode functions deprecated2024-01-11T18:31:16ZBradley TaylorPHP 8.2 utf8_encode and utf8_decode functions deprecatedIn PHP 8.2 (planned to be released this November) `utf8_encode` and `utf8_decode` functions will be deprecated.
https://wiki.php.net/rfc/remove_utf8_decode_and_utf8_encode
I don't see this causing too many problems for CiviCRM, and mos...In PHP 8.2 (planned to be released this November) `utf8_encode` and `utf8_decode` functions will be deprecated.
https://wiki.php.net/rfc/remove_utf8_decode_and_utf8_encode
I don't see this causing too many problems for CiviCRM, and most uses are in upstream packages and libraries. However there are some uses in core itself (especially `utf8_decode`) which should be replaced.https://lab.civicrm.org/dev/core/-/issues/4887Discussion / clarification on Api4 permissions2024-01-05T13:49:04ZRichDiscussion / clarification on Api4 permissionsI've been trying to [grok](https://en.wikipedia.org/wiki/Grok) this while working on Standalone, and it's raised a few questions for me. It looks like there are several nuances/quirks/gotchas. This is my attempt to document these. I'd re...I've been trying to [grok](https://en.wikipedia.org/wiki/Grok) this while working on Standalone, and it's raised a few questions for me. It looks like there are several nuances/quirks/gotchas. This is my attempt to document these. I'd really appreciate anyone chipping in to improve this understanding. I'll close this issue off once I understand things, hopefully after having opened other issues/PRs including for the docs.
See: [Developer docs on security](https://docs.civicrm.org/dev/en/latest/security/permissions/#apiv4)
API access is split between Coarse and Fine grained levels.
- Coarse is: `action::permissions()`, `api::authorize`, `AuthorizeEvent`,
`PermissionCheckSubscriber`...
- Fine is: `CoreUtil::checkAccessRecord`, `_checkAccess`, `AuthorizeRecordEvent`
**Coarse** level control is at the entity.action level only is handled by the
`API\Kernel->authorize()` method, which dispatches an `AuthorizeEvent` over
`civi.api.authorize`.
- `Api4/Event/Subscriber/PermissionCheckSubscriber` subscribes `W_LATE` and
authorizes the event: where checkPermissions is false or where the action is
getLinks, or if `$apiRequest->isAuthorized()` returns true. This generally is
handled by code in `AbstractAction` which finds the permissions needed
(`getPermissions()`) finds a match for the action name in the array returned
by the Api Action classes’ `permissions()` method) and checks them with
`\CRM_Core_Permission::check();`
Gotcha: the `save` action, if unspecified in the action's `permissions()`
method gets defaults from `create` (if specified) rather than `default`.
- there are other listeners too, (e.g. `AdhocProvider` (api4) and
`API/Subscriber/PermissionCheck` which is mostly api3 but it also authorizes
requests for api4 where `checkPermissions` has been set false, duplicating the
same logic above.)
**Fine** level control (at least for DAO entites) is at an Action-and-Record
level control (termed ACLs) rely on a single method on the entity's **BAO**:
public static function _checkAccess(string $entityName, string $action, array $record, ?int $userID): bool
- These methods are used by `\Civi\Api4\Utils\CoreUtil::checkAccessRecord`
- This only calls the BAO's `_checkAccess()` for actions that do not extend
`AbstractGetAction`; intended to be write actions, but could be others.
When it is called it must return TRUE for OK. A FALSE value will cause an
exception.
- `_checkAccess` is **not** called if a listener to
`\Civi\Api4\Event\AuthorizeRecordEvent` authorizes or explicitly declares
the event unauthorized.
- `_checkAccess` must only check data as follows: entity and action name and nothing else from `$apiRequest`. Data in `$record` (see below for what this contains). It may identify the record's primary key ID and fetch data from the database to consider, too.
- `checkAccessRecord` is used:
- `AbstractUpdateAction::_run` calls it per row to be updated
- `DAODeleteAction::_run` calls it per row.
- `BasicBatchAction::_run` calls it per row.
- `AbstractCreateAction::validateValues` calls it. (before `ValidateValuesEvent`)
### `$record` for Delete
This is `[$idFieldName => $id]` only.
### `$record` for Create
validateValues calls `_checkAccess` with data that has been pre processed via: *formatWriteValues » fillDefaults*
### `$record` for Update
- formatWriteValues
- some special code to convert passing a primary key value in values to a WHERE
clause, and to rule out the possiblity of changing an existing primary key
- For single row updates:
- populate only the row's id field (from the where)
- calls `_checkAccess` with the id field + input (after formatWriteValues)
- For batch updates:
- getBatchRecords
- each row, we take the input (after formatWriteValues) and fill from
existing row data. Then call `_checkAccess`.
- *then* call validateValues() which dispatches a ValidateValuesEvent
over `civi.api4.validate`
### `$record` for Save
- Loops rows
- applies defaults passed in to this specific row
- `formatWriteValues`
- `matchExisting` (presumably sets PKs?)
- if a create, fills defaults
- calls `validateValues` which loops records and for each calls
`checkAccessDelegated` with either 'create' or 'update'
- `checkAccessDelegated` creates a new API request of the same type and calls
`authorize`, as a way to check permissions for the entity.action pair.
(inside loop!)
- it then calls `_checkAccess` for the record with this new empty api request
and the record as a separate array. i.e. the `$apiRequest` handled by
`_checkAccess` only contains the entity name and action name.
## Questions
- There's a couple of places where I think we could reduce work that is
unnecessarily repeated in a loop.
- `AbstractSaveAction::validateValues` calls `checkAccessDelegated` which in
turn calls `authorize()` on a blank API call every time. The answer to this
won't change during the loop; there may be one answer for 'create' and one
for 'update'.
- `Generic\BasicBatchAction` (minor): move if(checkPermissions) to outside
the loop and cache the logged in user outside the loop too.
- `_checkAccess` is only called for actions that don't extend the
AbstractGetAction. This could potentially be confusing for other reporting
actions that don't extend this (maybe they should?). I've seen quite a few
examples where specific cases return FALSE and at the end there's a default
return TRUE. I'm not suggesting there's a security problem, but this doesn't
feel "security-first", i.e. it's a blanket allow with some exceptions rather
than a blanket deny with allow being explicitly checked.
- It feels weird that we start from an entity-action-specific class, then call
a wider entity-specific method, passing in the name of the action, meaning that
the logic then needs to disaggregate the action-specifics. It also feels weird
that `_checkAccess` is in the BAO, though it's only called by Api4 code.
Wouldn't it be better to have `_checkAccess` in the action classes?
- `$record` passed as part of update actions is inconsistent. It contains full original
data with updated field values replaced for *batch* updates, but only the
updated field values and primary key for *single* updates. Later,
validateValues is called with a `CRM_Utils_LazyArray` that would *reload* this
data (in the case of a batch update). The `ValidateValuesEvent->records` *only*
contains the updated values. So three possible different sets of data.
- A comment in the `ValidateValuesEvent` notes that it's expensive to lookup the
original values (because it would be one query per record), so avoid if possible.
The lazy array returns only the update values as 'new', not the merged array
(this is the same as in its records property). There is duplication for an
*update action* since these are already present on the event under 'records'
and are shared between all rows. However for a *save action*, there could be
per-row specific updates.
Q. where we've loaded the data (e.g. batch update) couldn't we make that available
to save the lazy function doing a one-query-per-row?RichRichhttps://lab.civicrm.org/dev/core/-/issues/4146Upgrade Smarty to Smarty3+2024-01-03T19:48:39ZeileenUpgrade Smarty to Smarty3+This is a meta issue for looking at moving from Smarty2 to Smarty3+
**Status as of 5.68 - Smarty3 is optional, It is recommended in 5.69 as the preferred version**
to enable - add a define to `civicrm_settings.php` like this (adjust...This is a meta issue for looking at moving from Smarty2 to Smarty3+
**Status as of 5.68 - Smarty3 is optional, It is recommended in 5.69 as the preferred version**
to enable - add a define to `civicrm_settings.php` like this (adjust sample path as desired)
```
if (!defined('CIVICRM_SMARTY3_AUTOLOAD_PATH')) {
define('CIVICRM_SMARTY3_AUTOLOAD_PATH', $civicrm_root . '/packages/smarty3/vendor/autoload.php');
}
```
**Gaps as of 5.68**
- [x] resolve Sections section of civi-report https://github.com/civicrm/civicrm-core/pull/27777
- [x] the online membership receipt has an instance of 'crmMoney maths' - this should be the same as other templates - I'm just struggling to understand the double contribution thing with separate membership payment
- [ ] not all extensions are compatible - see below
**Things that cause compatibility issues** (resolve in core with the exception of the 2 listed)
- **maths** in Smartyv2 {$amount+$taxAmount|crmMoney} is implicitly `{($amount+$taxAmount)|crmMoney}` - whereas in Smarty3 it does not resolve correctly without the braces - which Smarty2 does not support. Generally the maths can be moved to the php layer
- Extensions with CiviX versions lower than 23.01 - mostly these only cause notices & smarty files will not load - although some very old civix versions will cause a hard fail. Running civix will remove these lines
![image](/uploads/6ec4b8e83ac6fa02c1ec6d0bec57bac8/image.png)
And add in the smarty mixin if smarty files are in use
- Accessing the smarty property `_tplVars` directly (in most cases running civix will fix)
- usage of `{php}` within tpl files
- incorrect variable assignments in templates for correct quotes and backticks, e.g.
```
{assign var="foo" value="bar-`$something.else`"}
```
if a variable is being concatenated into a string in a template, the string should be surrounded by double quotes, not single. If the variable name contains characters other than a-zA-Z0-9_ (e.g. a period), then it needs to be surrounded by backticks. If a variable is an int and you're doing math, don't use backticks (e.g. `value=$one.thing+1`).
**Why smarty3**
1. Accessing documentation is less confusing as Smarty documentation targets v3+
2. Smarty3 is more secure - in part because it has security releases, in part because escaping is on by default - which actually still seems disabled in the working version - but it makes sense to upgrade smarty first
3. Smarty3 is more efficient in handling one-off strings. We have kinda hacked this into Smartyv2 with a combination of the core function `CRM_Utils_String::parseOneOffStringThroughSmarty` and hacking some security into `fetch` but under volume this can result in #4143 and also reads & writes to disk more than is ideal
4. The work to get to Smarty4 is the same as the work to get to Smarty3 (mostly done) + an unknown extra amount of work. Not a bad thing to do but a big scope creep
5. In theory `Smarty3` is a relatively [seemless upgrade from Smartyv2](https://github.com/smarty-php/smarty/blob/v3.1.47/SMARTY_2_BC_NOTES.txt).
**Things that need to be done in extensions**
Other than the first item these are fairly uncommong
1) run `civix upgrade` on extensions where [the civix version of the extension is 23.01 or lower](https://github.com/civicrm/civicrm-core/pull/27565#issuecomment-1732155043). (it's worth making sure you have the [latest civix first](https://github.com/totten/civix/tree/master)
4) test any unusual smarty screens
**Challenges outstanding**
1) When we are happy from a QA point ov view we need to roll it out as part of our package. At that point we will need to decide if it is opt in & what that looks like or whether it is a hard-switch.
** Challenges that we are doing OK on **
- **Autoloading** - works OK if extensions are updated to recent civix.
- **Function Naming** - Smarty3 has renamed a bunch of functions - eg `register_resource` becomes `registerResource`. This seems to be manageable with [a compatibility class](https://github.com/civicrm/civicrm-core/pull/27585) - allowing us to switch to the forward compatible function names. Note it really is mostly (maybe only) core that interacts with these functions
- **Fetch override** - We have overridden the `fetch` function to try to mimic Smartyv3 security in v2 - this function has a different signature in `v3` so overriding it is a noisy experience. [Solution is to move our patches to Smartyv2 packages](https://github.com/civicrm/civicrm-core/pull/27588)
- **Escape by default** - so far this seems to have not been enabled on Smarty3 - I think it's fine to defer worrying about it until the upgrade is done if it is not hurting us
- **Invalid Smarty Syntax** - so far I have identified that the use of '`' in Smarty causes a hard crash - we don't do this much, mostly in older code... @larssg has done a [pretty solid clean up in core](https://github.com/civicrm/civicrm-core/pull/27547) but there could be some in extensions
- **Direct access of class properties** - we have a couple of places that do things like `$smarty->_tpl_vars` - those need replacing with `get_template_vars or, if we add compatibility `getTemplateVars`
- **Register String Resource** - this is not an issue for `v3` but is the current blocker on `v4` the parameter type in our function `civicrm_smarty_register_string_resource` is an `array` but that is not v4-compatible
**Related**
https://lab.civicrm.org/dev/core/-/issues/4618https://lab.civicrm.org/dev/core/-/issues/4251Track Contact `image_URL` files in the `civicrm_file` table2024-01-02T21:02:33ZcolemanwTrack Contact `image_URL` files in the `civicrm_file` table# History
**Note: CiviCRM is a large, old project with many contributors, which makes "big picture" perspectives cumbersome to gather. Often a contributor simply wants to fix a bug or add a small feature, not dive into decades-long hist...# History
**Note: CiviCRM is a large, old project with many contributors, which makes "big picture" perspectives cumbersome to gather. Often a contributor simply wants to fix a bug or add a small feature, not dive into decades-long history and contemplate massive refactoring. The story of the `civicrm_contact.image_URL` field is a microcosm of the complicated world of CiviCRM.**
- The `civicrm_contact.image_URL` field (added v1.1) predates the `civicrm_file` table (added v1.5), which may explain why it was originally designed as a simple textfield with no file-management. It is a simple varchar and can store the url to any image file on the web.
- Originally, the UI allowed contact image files to be uploaded to a publicly-readable directory on the webserver, and `image_URL` stored an absolute url to that file.
- In 2014, [security hardening](https://issues.civicrm.org/jira/browse/CRM-14499) led to the addition of an `.htaccess` rule which blocked the contents of that directory from public visibility.
- This accidentally broke contact images, leading to [a rushed fix](https://github.com/civicrm/civicrm-core/pull/3058) which created `CRM_Contact_Page_ImageFile` at `civicrm/contact/imagefile`. This path allows open access to all contact images, but not other files, by querying `civicrm_contact.image_URL` for a matching filename before outputting the contents. An upgrade script rewrote all local `image_URL` fields to point to this path, using an absolute URL. This solution works but is very slow on big databases due to the unindexed query.
- During this rushed fix, it doesn't appear that consideration was given to html escaping of `&` characters. The default behavior of `CRM_Utils_System::url` is to escape `&` to `&` which is (IMHO) a bad default and certainly a poor choice for storing url strings in the database, but it was the default and no one changed it, and then Tim inadvertently cemented it with 065034510d48b722844b2007f8016ea644bd0cbd so now in order to safely read the urls you must pass them through the aptly-named `CRM_Utils_String::unstupifyUrl()` function.
- In 2016 there was an [attempt to fix the absolute URL and performance issues](https://github.com/civicrm/civicrm-core/pull/9237) which updated all the `image_URL` fields to relative paths.
- But this would have broken Drupal Views and other tools that rely on the convenience of querying the field from the db and outputting the url directly (in the future, SearchKit would rely on this too).
- A [compromise solution was reached](https://github.com/civicrm/civicrm-core/pull/9241) which rewrites the url at runtime on Civi pages. A `image_URL` like `http://wp.demo/civicrm/contact/imagefile/?photo=abc.jpeg` would get rewritten to `http://wp.demo/civicrm/file?reset=1&filename=abc.jpeg&mime-type=image/jpeg`. For reasons not entirely clear, this goes through a different internal path (`civicrm/file` instead of `civicrm/contact/imagefile`).
- In 2019, thinking it was unused, [Eileen removed support](https://github.com/civicrm/civicrm-core/commit/2762985c11e01ede473d86a33af279bc341f9a46) for passing `filename=` into the `civicrm/file` path, since that endpoint is typically supplied with a file id from the `civicrm_file` table plus a security hash.
- This accidentally broke contact images, leading to [a rushed fix](https://github.com/civicrm/civicrm-core/pull/13663) which added back the ability to get files by name from the `civicrm/file` path (since contact images are not tracked in `civicrm_file` and don't have an `id`). With the benefit of the history laid out above, a better fix might have been to switch to using the `civicrm/contact/imagefile` path and keep the `civicrm/file` path secure.
- In 2021 [I proposed adding](https://lab.civicrm.org/dev/core/-/issues/2755) an `image_file_id` FK field to the `civicrm_contact` table to track uploaded files. This proposal was met with approval, but when I recently tried to implement it I realized that the `civicrm_file` table already has an FK to `civicrm_contact` and circular references are not allowed.
- In 2023 [an option group was added](https://github.com/civicrm/civicrm-core/pull/25904) for the previously unused column `civicrm_file.file_type_id`. One possible use for that field would be to designate a file type of `"contact_image"`.
# Current Situation
The `civicrm_contact.image_URL` field can still store any url string pointing to a file on or off the server. It could point to any image on the internet, and would work fine. But if it's a file uploaded via the Civi UI, it will be an absolute link pointing to the `civicrm/contact/imagefile` path with a `photo=filename` argument. If CiviCRM recognizes this pattern it will rewrite it on core pages to the other path at `civicrm/file`, otherwise it will leave it alone.
For the confused, yes contact images are accessible at two paths, and neither is a direct link to the file on disk:
| Path | `civicrm/contact/imagefile` | `civicrm/file` |
| ------ | ------ | ------ |
| Class | `CRM_Contact_Page_ImageFile` | `CRM_Core_Page_File` |
| Permission | _none_ | "access uploaded files" |
| Args | `photo` | `filename`, `mime-type` |
| Uses | Stored in `image_URL` field as absolute URL. Output by Views & SearchKit | `image_URL` rewritten to this path on Civi pages for logged-in users |
**This situation leads to the following quirks and problems**
1. The absolute url to `civicrm/contact/imagefile` works great in Views and SearchKit... as long as the site name never changes! Otherwise, absolute URLs are a pain.
2. The url is still stored with html-escaped `&` characters that must be unstupified.
3. Anyone can access a contact image via the 1st path if they know the filename, however, the 2nd has a permission check which means logged-in users without "access uploaded files" cannot see contact images even though anonymous users can!
4. The security hash usually required by `civicrm/file` can be circumvented if you know the filename and mime-type. But the risk is mitigated by that path requiring "access uploaded files" permission.
5. There is still no file-management of contact images. Deleting a contact does not delete their image file. Deleting or changing a contact image also doesn't delete the old one.
# Proposal for File Management
1. Stop using `civicrm/file` for all contact images and [restore the patch to remove support for `filename`](https://github.com/civicrm/civicrm-core/commit/2762985c11e01ede473d86a33af279bc341f9a46).
2. Include `cid` as an argument to `civicrm/contact/imagefile` (and update stored paths accordingly) to fix the unindexed query. Also add `is_deleted = 0` to the query.
3. Add an option_value `"contact_image"` to the option group for `civicrm_file.file_type_id`.
4. When uploading a new contact image, create a record in `civicrm_file` table, and designate it `file_type_id` = `"contact_image"`.
5. Also create a record in `civicrm_entity_file` for contact images.
6. Add a virtual APIv4 field for the contact entity `image_file_id` which would allow getting/setting the file id. When setting a new file id, regenerate the `image_URL` with a post hook.
# Thoughts on Absolute URLs
All of these changes would result in better file management, but still doesn't solve the absolute url issue. This is tricky to solve because Views and SearchKit still rely on being able to output the image url directly from a query. Here are a few ideas for that one:
1. Bite the bullet and update all `image_URL` fields pointing to a local file to use a relative URL. SearchKit will still work. Views and other SQL-based tools will still work unless embedded on a remote site. Random offsite images will be unaffected.
2. Keep `image_URL` absolute but add an APIv4 virtual field like `Contact.image` which calculates the url at runtime. This satisfies moree use-cases but at the expense of adding complexity to an already overcomplicated situation.https://lab.civicrm.org/dev/core/-/issues/4357Timezone with Drupal 9, CiviCRM entity and Views2023-12-23T00:55:25Z5knotsTimezone with Drupal 9, CiviCRM entity and ViewsOverview
----------------------------------------
We are working with grants, where fields like "money transferred" exist. This field is stored in the database as a "date-only" value (screenshot). However, Drupal interprets the date as a...Overview
----------------------------------------
We are working with grants, where fields like "money transferred" exist. This field is stored in the database as a "date-only" value (screenshot). However, Drupal interprets the date as a datetime field and, presumably, sets the time to 12 noon, but a day before. This leads to inconsistent dates in Drupal Views outputs. Is there a known workaround to avoid this issue? Currently, I have set a fixed 12-hour timezone difference for the View, which works but is not an ideal solution.
Reproduction steps
----------------------------------------
1. Install civicrm entity and enable grants
1. go to Drupal Views and create a new view based on grants.
1. include fields like 'money transferred'
Current behaviour
----------------------------------------
Drupal interprets the date as a **datetime** field.
Both attached screenshots are from the same entry.
![Bildschirmfoto_2023-06-13_um_14.41.49](/uploads/6b7df46f1fdd9354238c4e4e90ce0dec/Bildschirmfoto_2023-06-13_um_14.41.49.png)
![Bildschirmfoto_2023-06-13_um_14.43.32](/uploads/e324ce91d7fe2a48a0ce84c4bb580589/Bildschirmfoto_2023-06-13_um_14.43.32.png)
Expected behaviour
----------------------------------------
Drupal interprets the date as a **date** field.
Environment information
----------------------------------------
D9.5, CiviCRM 5.62, civicrm_entity /latesthttps://lab.civicrm.org/dev/user-interface/-/issues/49Search Kit builder UX improvements2023-12-21T22:33:04ZnicolSearch Kit builder UX improvementsFollowing discussion at the sprint with @gibsonoliver, @davem, @colemanw, @christhompson, @callanpaterson {please add others} – am aggregating some of the feedback.
**What this isn't**. A proposal for a UX rewrite, which would normally ...Following discussion at the sprint with @gibsonoliver, @davem, @colemanw, @christhompson, @callanpaterson {please add others} – am aggregating some of the feedback.
**What this isn't**. A proposal for a UX rewrite, which would normally start with a whole process of user feedback & profiling, workshoping, wireframing and so on which is a whole different proposal.
**What this is.** Low hanging fruit, ie optimisations that could make this interface more intuitive, especially for people not familiar with mysql builder, or more used to Drupal Views syntax.
Proposals are grouped in four areas:
1. **Language**. For e.g. changing '_Where_' to '_Filter_', and '_Having_' to '_Filter within Results_', which would make clear both do similar things (filter), but how they are different.
2. **Help tooltips**. An (i) icon that can be clicked for a quick explanation of a term, plus a link to the docs where possible (ie 'WITH is a connection to another entity, like a MySQL JOIN or Views RELATIONSHIP. Learn more >>')
3. **Pre-built Reports** as Search Kit displays in the UI out-the-box. ie recreate some common CiviReports as SK displays that people can then tweak/clone/deconstruct, and in doing so learn a bit of the interface/structure. Inspired by the comment 'Tweaking is easier than creating'.
4. **Reorder elements** of the page to make the user journey more obvious. These should be doable either through CSS grid reordering, or minimal markup tweaks. E.g. (brainstormed ideas not proposals):
- ensure all related blocks are within a parent visual container/border to keep them separate when queries get longer,
- put some elements (ie group or query info) behind an 'advanced' expanding region,
- move left column of searches and displays into top tabs to increase available page width,
- rethink two columns in favour of list of blocks, similar to advanced search.
In addition @artfulrobot raised the issue of adding descriptive class names to regions to make it easier to target them.https://lab.civicrm.org/dev/core/-/issues/1083Non-deductible amount not working2023-12-21T11:11:54ZJoeMurrayNon-deductible amount not workingOn 5.14 and dmaster 5.16.alpha1 when a price set is configured with an option that includes a non-deductible amount, then a purchase is made of that option, the non-deductible amount is not appearing on the contribution.
Price Set Field...On 5.14 and dmaster 5.16.alpha1 when a price set is configured with an option that includes a non-deductible amount, then a purchase is made of that option, the non-deductible amount is not appearing on the contribution.
Price Set Field with non-deductible $5 on $10 price:
![2019-06-27_14-44-40](/uploads/3954b396bb81d67c5f01cf5c6464b85a/2019-06-27_14-44-40.png)
Contribution for $10 with $0 non-deductible:
![2019-06-27_14-43-59](/uploads/9c43a1a6c2a0381ed94c403901228821/2019-06-27_14-43-59.png)EdselopezEdselopezhttps://lab.civicrm.org/dev/core/-/issues/4845CiviEvent with approvals: Registration form not pre-filled with (custom) part...2023-12-20T19:21:06ZTobias Voigttobias.voigt@civiservice.deCiviEvent with approvals: Registration form not pre-filled with (custom) participant dataI'm not sure wether this is a proposal or an issue. I'm using the approval workflow for CiviEvent where participants get a confirmation link after they've been approved by admins.
While this works great for simple registration use cases...I'm not sure wether this is a proposal or an issue. I'm using the approval workflow for CiviEvent where participants get a confirmation link after they've been approved by admins.
While this works great for simple registration use cases, where only contact information is gathered in the initial registration form, this feature regrettably can't be used for more comlex use cases, where the initial form also contains additional (custom) participant data (e.g. "Do you want the vegetarian option for dinner?").
So, if there's any fields for custom participant data in the initial registration form, users would fill out the form and send it. Afterwards, admins approve the registration and a confirmation link is sent out to the approved user. If he/she clicks on the confirmation link and chooses to confirm, all contact data gets pre-filled into the form - BUT: the participant data - which is already stored in the database from the initial form - is not pre-filled and has to be filled out again.
This drastically limits the scope of use cases this feature could work for, since in almost all our client's projects we work with additional / custom participant data.
So my question is: Is this a bug and the participant data should actually be loaded during the confirmation process? Or was this feature never intended? Or maybe the feature was intended but never realized? In any case: What might be the scope of development work necessary to not only pre-fill the confirmation form with contact data but also with participant data?https://lab.civicrm.org/dev/core/-/issues/4832CiviReport 'Available for Dashboard' checkbox lost functionality2023-12-20T19:20:26ZbrienneCiviReport 'Available for Dashboard' checkbox lost functionalityOverview
----------------------------------------
The ‘Available for Dashboard?’ checkbox on CiviReports has lost its functionality.
Reproduction steps
----------------------------------------
1. On an existing or new CiviReport, click ...Overview
----------------------------------------
The ‘Available for Dashboard?’ checkbox on CiviReports has lost its functionality.
Reproduction steps
----------------------------------------
1. On an existing or new CiviReport, click on the **Access** tab
* If it is a new report, add a title on the **Title and Format** tab to make sure the report can be easily found.
1. Check the box next to the *Available for Dashboard?* label
1. Save the report from the *Actions* menu.
1. Go to the CiviCRM home page and do a hard reload (or even a cache clear)
1. Note that the report is either still present as an available dashlet ~~or not listed at all.~~
Current behaviour
----------------------------------------
Unchecking the *Available for Dashboard?* box on an existing CiviReport that is already available as a dashlet does not remove the report from the list of available dashlets on the CiviCRM home page
~~Checking the *Available for Dashboard?* on a new or existing CiviReport does not add it to the list of available dashlets on the CiviCRM page.~~
Deleting a report also does not remove the dashlet from the available options, however, the user will encounter a non-fatal error message that ‘You have tried to access a report that does not exist.’
Expected behaviour
----------------------------------------
The *Available for Dashboard?* checkbox should accurately control whether the report is or is not available as a dashlet.
Environment information
----------------------------------------
* __CiviCRM:__ 5.67.0https://lab.civicrm.org/dev/core/-/issues/4846API Deprecation2023-12-20T19:19:58ZtreseroAPI DeprecationLots of these in the log for mailings
[warning] Deprecated function CRM_Mailing_BAO_MailingComponent::retrieve, use API.
CRM_Core_Error::deprecatedFunctionWarning
CRM_Mailing_BAO_MailingComponent::retrieve
CRM_Mailing_Form_Component::se...Lots of these in the log for mailings
[warning] Deprecated function CRM_Mailing_BAO_MailingComponent::retrieve, use API.
CRM_Core_Error::deprecatedFunctionWarning
CRM_Mailing_BAO_MailingComponent::retrieve
CRM_Mailing_Form_Component::setDefaultValues
CRM_Core_Form::buildForm
Array
(
[civi.tag] => deprecated
)
Trying to make an email template, and I get this.
Wish I could help more.https://lab.civicrm.org/dev/core/-/issues/4871Afform - Reset button click does not refresh the search result table with emp...2023-12-20T19:19:39ZjitendraAfform - Reset button click does not refresh the search result table with empty filtersTo replicate:
- Create a search kit to search for contacts.
- Add a form builder with a filter input Display Name. Also enable Reset button.
- View the search form and search for display name:
![image](/uploads/61162fab96405b82341a39ddb...To replicate:
- Create a search kit to search for contacts.
- Add a form builder with a filter input Display Name. Also enable Reset button.
- View the search form and search for display name:
![image](/uploads/61162fab96405b82341a39ddb461d9d6/image.png)
- Click Reset and search again.
**Expected**: The search refreshes the table with empty displayname and returns all contacts.
**Actual**: The search does not refresh the table.
![image](/uploads/7b807b8aee9ee97d3b8c5b5c98a133aa/image.png)
Note: If i clear the textbox manually and hit search, the search is working fine.https://lab.civicrm.org/dev/core/-/issues/4868Add action for copy activity2023-12-20T19:16:21ZyashodhaAdd action for copy activityWe do have copy action on various entities like _Contribution Page_, _Events_.
It might be handy to have the same for _Activity _as well.
Also provide _Save as New_ feature/ when clicked we can have a new activity created with addition...We do have copy action on various entities like _Contribution Page_, _Events_.
It might be handy to have the same for _Activity _as well.
Also provide _Save as New_ feature/ when clicked we can have a new activity created with additional tweaks based on a pre-existing activity. This would come esp handy for activities which have lots of Activity Contacts/Targets/assignees and custom dat awith the original activity acting as a template.yashodhayashodhahttps://lab.civicrm.org/dev/core/-/issues/4865The "X" on advanced search accordions isn't doing the right thing anymore2023-12-20T19:15:35ZDaveDThe "X" on advanced search accordions isn't doing the right thing anymoreFollowup to https://lab.civicrm.org/dev/core/-/issues/4856
It looks like it's been a problem since at least 5.63.
What used to happen was if you just collapsed the accordion, it still used whatever search criteria had been entered (bec...Followup to https://lab.civicrm.org/dev/core/-/issues/4856
It looks like it's been a problem since at least 5.63.
What used to happen was if you just collapsed the accordion, it still used whatever search criteria had been entered (because after the search the form still needs to retain the search criteria even with them collapsed), and in fact even just opening the accordion would make it do the table joins (e.g. if you open the contributions accordion it's the same as "only show me contacts who have at least one contribution"). That's all working now again after 4856. But the "X" at the far right of the accordion header was there to completely remove the criteria and table joins from the query. That's not working anymore, and broke before any accordion changes.
It is _sort of_ working in the sense that it will clear contribution date for example, but not some other fields like id or transaction. And in both cases the accordion still pops open after search. Also note the defaults disappear so it's now including test and template contributions.
And additionally, for transaction, if you repeat and close the accordion again and click search again, you get a crash because it tries to pass array for some reason to mysqli_escape_string.https://lab.civicrm.org/dev/core/-/issues/3313Unable to save a membership with a text price field2023-12-19T22:18:09ZDaveDUnable to save a membership with a text price fieldSame problem in 5.28 and master and can reproduce on dmaster.demo.civicrm.org. Or maybe I'm missing something.
1. Create a price set used for memberships.
1. Set financial type to member dues.
1. Add one text field (the default input fi...Same problem in 5.28 and master and can reproduce on dmaster.demo.civicrm.org. Or maybe I'm missing something.
1. Create a price set used for memberships.
1. Set financial type to member dues.
1. Add one text field (the default input field type).
1. Amount doesn't matter - put in some number.
1. Leave other fields at defaults.
1. Create a new membership.
1. Choose the price set.
1. Put in quantity 1.
1. Can leave the rest at defaults.
1. Click Save.
1. ERROR: Select at least one membership option.
It works with a radio field.https://lab.civicrm.org/dev/core/-/issues/4857Standalone: cms:administer users permission is not declared; cannot be granted2023-12-19T11:20:09ZRichStandalone: cms:administer users permission is not declared; cannot be grantedThere's existing core code for `cms:administer users` permission which standalone has also used for various forms/apis.
But it doesn't exist.
It may be as simple as declaring the permission in a hook in standaloneusers ext. Or it may n...There's existing core code for `cms:administer users` permission which standalone has also used for various forms/apis.
But it doesn't exist.
It may be as simple as declaring the permission in a hook in standaloneusers ext. Or it may need more work.https://lab.civicrm.org/dev/core/-/issues/4813CRM_Report_Form_Activity: Add Employer field, and "contact"-based custom fields2023-12-18T23:06:40ZAllenShawCRM_Report_Form_Activity: Add Employer field, and "contact"-based custom fieldsThis improvement is requested and sponsored by Stuart at Korlon.
We aim to implement the following changes to the CiviCRM core "Activity Detail" report (CRM_Report_Form_Activity):
- add "Current Employer" as an available column (not fi...This improvement is requested and sponsored by Stuart at Korlon.
We aim to implement the following changes to the CiviCRM core "Activity Detail" report (CRM_Report_Form_Activity):
- add "Current Employer" as an available column (not filter), to display with its Display Name, as a link to the employer contact
- auto-include "contact"-based custom fields (currently only "individual"-based custom fields get this treatment)
A PR is forthcoming. If it seems this is not a desirable improvement, please let me know in comments!
(Joinery reference: F#1317)