CiviCRM Core issueshttps://lab.civicrm.org/dev/core/-/issues2022-09-27T23:42:48Zhttps://lab.civicrm.org/dev/core/-/issues/1328Escape-on-Input => Escape-on-Output2022-09-27T23:42:48ZtottenEscape-on-Input => Escape-on-Output# Introduction
In a multi-tier application like CiviCRM, a piece of data (such as a `first_name`) may be passed through several systems and formats (MySQL/PHP/Firefox/etc; SQL/HTML/URL/JSON/CSV/PDF/etc). In each system, this piece of d...# Introduction
In a multi-tier application like CiviCRM, a piece of data (such as a `first_name`) may be passed through several systems and formats (MySQL/PHP/Firefox/etc; SQL/HTML/URL/JSON/CSV/PDF/etc). In each system, this piece of data may be encoded in a different way. The differences are sometimes imperceptible to a casual reader (ex: "banana" looks the same in SQL, HTML, URL, JSON, and CSV), but mistakes and confusion about encoding are still problematic. The impact ranges from quirky (odd characters appearing occasionally in unexpected places) to malicious (security vulnerabilities).
CiviCRM has a long-standing technical debt with respect to encoding. This debt is not specifically a bug, feature, or vulnerability in itself. (If you know a specific piece of data that is misencoded, then that is often a security problem that should be [reported to the security team](https://civicrm.org/security) for discrete resolution.) Rather, this debt is *an unusual convention* ("Escape-on-Input") which has *unintuitive consequences* and thereby invites bugs.
See also: [Dev Guide: Sanitize inputs or outputs?](https://docs.civicrm.org/dev/en/latest/security/#strategy)
At the recent Barcelona sprint, there was a partial meeting of the security team, and attendees agreed that this was the root issue behind multiple problems and merited special attention. Unfortunately, this convention is deeply embedded. Changing it is difficult and risky, so it has lingered for several years without substantive improvement, and (with its current framing/visibility) would remain indefinitely. The aim of this filing is to have a public discussion/record which works out the what/how/why of a (possible) cleanup.
# Definitions
* __Escape-on-Input (aka Esc-In)__: An architectural style in which data is taken as input and immediately escaped; it is then written to disk in escaped (HTML-esque) format. Consequently, subsequent searches+reads yield HTML-esque strings. These can be outputted literally on HTML pages without any extra processing, but (for other media) they require double processing (decode/re-encode).
* __Escape-on-Output (aka Esc-Out)__: An architectural style in which data is is taken as input and stored in its canonical format. When the data is presented as output, it is escaped in a way that matches the current output-format.
* __Canonical (String format)__: An encoding in which strings look... like they should. For example, if a user types `first_name` of `:<`, then the string is `:<`
* __HTML-esque (String format)__: An encoding in which key HTML characters are escaped as entities. For example, canonical string `:<` becomes HTML-esque string `:<`. In HTML-esque, one does not intend to convey HTML tags.
* __O(1) Plan/Task__: A plan/task in which the number of tasks is fixed. (Ex: To change behavior of all "quickforms", you might patch+test the base class `HTML_Quickform` one time.) It may strictly be 1 or 2 or 3 steps... but the number of steps does *not* depend on how many screens/use-cases are impacted.
* __O(n) Plan/Task__: A plan/task in which the number of tasks depends on how many screens/use-cases are impacted. (Ex: To change behavior of all "quickforms", you might patch+test *every subclass*.)
* __Smarty Filter/Autoescape__: A Smarty filter allows one to programmatically change the behavior/content of all Smarty templates. For example, you can write a Smarty prefilter to automatically add HTML "escape" commands to all output.
* __Linter__: A programmer tool which scans source code and checks for compliance. For example, a linter could have the rule "all Smarty variable include the `|escape` or `|noescape` modifier."
# Safe and unsafe situations
The status quo is peculiar. It is loosely safe in some common situations, but it's surprisingly unsafe in other situations. Consider this table:
<table>
<thead>
<tr><th></th><th>Read/write data via SQL/DAO/BAO</th><th>Read/write data via APIv3/v4</th></tr>
<thead>
<tbody>
<tr><th>Process inputs+outputs via HTML_Quickform+Smarty (HTML-only)</th><td>OK</td><td>Caution</td></tr>
<tr><th>Process inputs+outputs via most frameworks (Drupal Form API, AngularJS, DOM API, PHPExcel, etc)</th><td>Caution</td><td>OK</td></tr>
<tr><th>Interchange data between SQL/DAO/BAO and APIv3/v4</th><td>Caution</td><td>Caution</td></tr>
</tbody>
</table>
In "OK" situations, you can take a string and send it to the screen -- and there's a high probability it will end-up encoded correctly without any thinking/effort.
In "Caution" situations, you should treat most strings with suspicion -- many would merit extra encoding/decoding steps.
The general aim of this cleanup issue is to be "OK" on the entire grid.
# Relevant background/reading
* (Jan 2010) [CRM-5667](https://issues.civicrm.org/jira/browse/CRM-5667): Introduction of "Escape-on-Input"
* (Dec 2012) [svn log](https://github.com/civicrm/civicrm-svn/commits/v4.2/CRM/Core/HTMLInputCoder.php): Mitigation for APIv3 consumers
* (Dec 2012) [CRM-11532](https://issues.civicrm.org/jira/browse/CRM-11532): First filing about the problematic status-quo. (Shorter)
* (Apr 2018, et al) [security/core#6](https://lab.civicrm.org/security/core/issues/6): Second filing about the problematic status-quo. (Longer)
* (2018) [Dev Guide: Sanitize inputs or outputs?](https://docs.civicrm.org/dev/en/latest/security/#strategy): Reference material for developers
* (Oct 2019) [POC: Auto-escaping in Smarty v2](https://gist.github.com/totten/5b30e10a21fe626a43a30e21ded26fc4)
* (Oct 2019) [Report: Categorize DB fields by escaping implication](https://gist.github.com/totten/ec78dcb869aa7cbbc95c5f273f7ea30d)
* (Oct 2019) [Report: List of Smarty expressions](https://gist.github.com/totten/9c7176bbdfc63f88d423f026359f50fc)
# Candidate Approaches
* __Do Nothing__:
* __How__: Kick off your shoes. Pull up Netflix.
* __Pro__: Less work
* __Con__: Encourages developers to write buggy code. Feels embarrassing whenever they realize this is a thing.
* __Epic w/Smarty Filter__:
* __Pitch__: Smarty does all HTML-escaping automatically and by default.
* __How__:
* Remove Esc-In policy (`HTMLInputCoder`)
* Write a generic upgrade step to recode all stored strings.
* To restore output-coding, add a Smarty prefilter to auto-escape output.
* __Pro__: Cleans up the database.
* __Con__: The main problem is Smarty templates are not homogeneous - *most* generate HTML, but *some* generate other formats. They *often* pass-through data from the database, but they *also* display data which is computed and/or loaded from a different source. Consequently, there is an O(n) task of re-testing all screens/fields and patching a large number of exceptions.
* __Con__: Additionally, there's the `r-tech` issue - even if we correctly change every screen/field/line in the main application, there is an open-set of third-party integrations. If these use SQL/DAO/BAO and work correctly in the current system, then they'll likely break as soon as the epic change is made, which will create stress for upgrade-scheduling.
* __Con__: Smarty is difficult to write unit tests for.
* __Epic w/DAO Filter__:
* __Pitch__: DAO does all HTML-escaping automatically and by default.
* __How__:
* Remove Esc-In policy (`HTMLInputCoder`)
* Write a generic upgrade step to recode all stored strings.
* To restore output-coding, update DAO/BAO with an optional escaping mode. (Take the current skiplist - everything else defaults to autoescaping.)
* __Pro__: Cleans up the database.
* __Pro__: Looks like an O(1) plan.
* __Pro__: Restoring output coding can be done at a single point - no need to fix existing templates.
* __Pro__: Output coding can be unit-tested.
* __Con__: Only addresses DAO-based query-building (`$d=new DAO_Foo(); $d->bar=123; $d->fetch()`). Does not address `executeQuery('SELECT foo AS bar...')`, `executeQuery('UPDATE foo SET bar...')`, or other query-building. This seems to be left as O(n) task?
* __Question__: Do we get read/write consistency?
* __Wait by Extension Approach__:
* __Pitch__: Grow new UIs on top of APIv3/v4. Obsolete everything all UIs that rely on DAO/BAO/SQL.
* __How__: Do nothing specifically for this problem. Instead, wait for extensions like Afform or Data Processor to gradually replace the HTML_Quickform/Smarty UIs. Once there's a critical mass, remove all old UIs and do an "Epic" switch.
* __Pro__: Don't have to do anything now! Creates pressure to do other useful changes.
* __Con__: You may be waiting a long time.
* __Incremental, Per-Component/Per-Field__:
* __Pitch__: Convert fields incrementally
* __How__: Make a list of all relevant DB fields (~500). In the first month, take a subset (such as "all CiviEvent fields") and transition/test the subset. In the second month, do another subset of fields (such as "all CiviCampaign fields").
* __Pro__: This allows tasks to spread out over time with more manageable/affordable chunks.
* __Con__: This still has the `r-tech` issue for third-party integrations. It creates a long-term drip-drip of compatibility breaks -- every month, some subset of fields have their data-format changed. This will make it quite challenging for extensions/integrations to keep in sync.
* __Comment__: Bespoke screens (e.g. "New participant") are amenable to simple grep+divide+conquer, but we might get boxed-in with how to update metadata-driven screens which tend to treat all strings the same way (e.g. import/export).
* __Incremental, Setting Plus Smarty Linting__:
* __Pitch__: Add toggle to allow old+new storage-formats. Incrementally enhance code to make new-storage-format work.
* __How__:
* Define a new setting to indicate whether strings are stored as "html-esque" (initial-default; long-term-deprecate) or "canonical" (initial-experimental; long-term-goal).
* Define a migration script - which can be run whenever one changes the setting.
* Update Smarty/Quickform/API to check the setting and filter accordingly. Define corresponding Smarty filter. (Naive example: `{$records[$cid]->display_name|crmEscape:'Contact.display_name'}` means "escape this data based on the escaping-rules for the `Contact` field `display_name`)
* Write a linter to report on which Smarty templates use the filter. Over time (month-by-month), incrementally update Smarty templates to use the new filters and re-test those screens.
* __Pro__: This allows tasks to be spread out over time with more manageable/affordable chunks. The linter provides a guide for helping us track "cleared ground".
* __Pro__: Site-implementers and extension/integration-authors will have greater flexibility on scheduling the change - they don't have to update all codes and all databases at the same time.
* __Con__: Requires exposing some new filters/functions/settings.
* __Incremental, Setting Plus DAO Filter__:
* __How__:
* Define a new setting to indicate whether strings are stored as "html-esque" (initial-default; long-term-deprecate) or "canonical" (initial-experimental; long-term-goal).
* Define a migration script - which can be run whenever one changes the setting.
* Update DAO/API to check the setting and filter accordingly.
* __Pro__: At a glance, this looks like an O(1) plan.
* __Pro__: Site-implementers and extension/integration-authors will have greater flexibility on scheduling the change - they don't have to update all codes and all databases at the same time.
* __Con__: Only addresses DAO-based query-building (`$d=new DAO_Foo(); $d->bar=123; $d->fetch()`). Does not address literal queries (`executeQuery('SELECT foo AS bar...')`, `executeQuery('UPDATE foo SET bar...')`) or other forms of query-building (`CRM_Utils_SQL`/APIv4/adhoc PHP). This seems to be left as O(n) task?
* __Question__: Do we get read/write consistency?
* __Incremental, SQL Views with Deprecation Alerts__:
* __Pitch__: Support both formats in MySQL *at the same time*. Borrow the idea of wide-spread VIEWs from multilingual.
* __How__:
* Every entity (eg `Contact`) should have a concrete TABLE and a derived VIEW (eg `civicrm_contact` and `civicrm_contact_new`) which present the same data in different formats (old/HTML-esque and new/canonical).
* `executeQuery()` generates a deprecation warning whenever there's a query that hits old-format table. The warning says something like "`civicrm_contact_old` is deprecated. Please use `civicrm_contact_new` instead. Be sure to update the escaping on any consumers."
* At the mid-point during transition, swap the SQL schema (eg the old-format is given by SQL VIEW and the new-format is given by SQL TABLE).
* Eventually, remove the old-format VIEWs completely.
* __Pro__: Existing (unpatched) consumers in any medium continue to work throughout the transition. But they start to emit warnings immediately.
* __Pro__: Extension/integration-authors (and site-admins/upgraders) have greater flexibility on scheduling changes - they don't have to update all codes and all databases at the same time.
* __Con__: Query performance on the filtered VIEW should be slower than the canonical TABLE. During the transitional process, queries will flop-flop around between faster/slower.https://lab.civicrm.org/dev/core/-/issues/1576Patches in `composer.json` do not apply cleanly2023-01-13T06:18:05ZtottenPatches in `composer.json` do not apply cleanlyOverview
----------------------------------------
The definition of `composer.json` appears to require certain patches; however, these cannot be applied in projects where `civicrm-core` is loaded as a dependency.
Reproduction steps
----...Overview
----------------------------------------
The definition of `composer.json` appears to require certain patches; however, these cannot be applied in projects where `civicrm-core` is loaded as a dependency.
Reproduction steps
----------------------------------------
```
drush dl drupal-8.7.x
cd drupal-8.7.x-dev
## Tangential workarounds
composer update psr/log
composer require 'pear/pear_exception:1.0.1 as 1.0.0' 'cache/integration-tests:dev-master#b97328797ab199f0ac933e39842a86ab732f21f9'
## Enable patching
## https://github.com/cweagans/composer-patches#allowing-patches-to-be-applied-from-dependencies
composer require cweagans/composer-patches:^1.5.0
composer config extra.enable-patching true
## Add CiviCRM
composer require civicrm/civicrm-core:5.22.x-dev
```
Current behaviour
----------------------------------------
As part of the output from `composer require ...` / `composer install`, we get:
```
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
- Installing zetacomponents/mail (dev-master 4dc71cc): Cloning 4dc71ccbcc from cache
- Applying patches for zetacomponents/mail
tools/scripts/composer/patches/civicrm-custom-patches-zetacompoents-mail.patch (CiviCRM Custom Patches for ZetaCompoents mail)
Could not apply patch! Skipping. The error was: The "tools/scripts/composer/patches/civicrm-custom-patches-zetacompoents-mail.patch" file could not be downloaded: failed to open stream: No such file or directory
...
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
- Installing phpoffice/common (0.2.9): Loading from cache
- Applying patches for phpoffice/common
tools/scripts/composer/patches/phpoffice-common-xml-entity-fix.patch (Fix handling of libxml_disable_entity_loader)
Could not apply patch! Skipping. The error was: The "tools/scripts/composer/patches/phpoffice-common-xml-entity-fix.patch" file could not be downloaded: failed to open stream: No such file or directory
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
- Installing phpoffice/phpword (0.15.0): Loading from cache
- Applying patches for phpoffice/phpword
tools/scripts/composer/patches/phpword-libxml-fix-global-handling.patch (Fix handling of libxml_disable_entity_loader)
Could not apply patch! Skipping. The error was: The "tools/scripts/composer/patches/phpword-libxml-fix-global-handling.patch" file could not be downloaded: failed to open stream: No such file or directory
```
Additionally, if you manually inspect the files `vendor/zetacomponents/mail/src/parser/interfaces/part_parser.php` and `vendor/phpoffice/common/src/Common/XMLReader.php`, you can see that the files appear unpatched.
Expected behaviour
----------------------------------------
The patches should apply.
Alternatively, if the patches are not needed, then perhaps we should remove them?
Environment information
----------------------------------------
* __PHP:__ 7.2.8
* __CMS:__ Drupal 8.7
* __Database:__ MySQL 5.7https://lab.civicrm.org/dev/core/-/issues/1615Migrate installers to "setup" API2024-02-07T01:05:15ZtottenMigrate installers to "setup" APIOverview
----------------------------------------
The aim of this filing is to do a round of consolidation/reconciliation in the Civi installer code: get rid of `install/*.php` and instead use `civicrm-setup`. The latter offers:
* Bett...Overview
----------------------------------------
The aim of this filing is to do a round of consolidation/reconciliation in the Civi installer code: get rid of `install/*.php` and instead use `civicrm-setup`. The latter offers:
* Better APIs and better organization - e.g. you can drill-down on most installation tasks by browsing the [plugins](https://github.com/civicrm/civicrm-setup/tree/master/plugins/) (esp `installFiles` and `installDatabase`); inspect the list of event-listeners; and [programmatically manage plugins](https://github.com/civicrm/civicrm-setup/blob/master/docs/plugins.md).
* More sensible UX - the list of options is more useful for a new admin initializing the system.
![Screen_Shot_2020-02-14_at_6.03.12_PM](/uploads/f107602f3ebd5f60b6870322711629a5/Screen_Shot_2020-02-14_at_6.03.12_PM.png)
Some background: https://civicrm.org/blog/totten/developers-revising-the-civicrm-installer-library-cli-wordpress
Use-cases
----------------------------------------
There are several different use-cases for performing installation; most permutations of the following variables are fair:
1. __UF/CMS__: Civi can be installed on top of Backdrop, Drupal 6/7/8, Joomla, WordPress.
2. __Medium__: Civi can be installed through:
* Web installation screens (e.g. `civicrm/install/index.php` and the Joomla installer)
* CLI installers (e.g. `cv core:install`)
* Scripts/packagers (e.g. Drupal "profiles", `civibuild`, `docker`)
3. __Options__: Civi can be configured with:
* CMS DB or separate DB
* Empty data or demo data
* "Components" (enabled/disabled)
* "Extensions" (enabled/disabled)
* "Settings" (defaults/overrides)
* Path/URL definitions (defaults/overrides)
Current behavior
----------------------------------------
There are separate installers. There are number of arbitrary differences and un-DRY things.
Proposed behavior
----------------------------------------
All installers use a PHP API for Civi installation.
Process
----------------------------------------
There are several things to consolidate here; it's not just one commit or PR. Suggested tasks (in approximate order):
* [x] __Move code from `civicrm-setup.git` into `civicrm-core.git`__ (*done, [#16618](https://github.com/civicrm/civicrm-core/pull/16618)*):
* `civicrm-setup` started as a separate repo so that I could iterate quickly in the early stages. However, as this gets rolled into more use-cases, it's likely to get referenced in more places (e.g. `civicrm-drupal-8.git`, `civicrm-drupal.git`, `civicrm-wordpress.git`, `civicrm-backdrop.git`), and it is also reasonable to expect a few small patches. Unfortunately, juggling small patches in that arrangement would be mentally draining. Migrating the code into `civicrm-core.git` will remove one layer of complexity.
* Like the API framework or DB framework, the *installer* is sufficiently important to `civicrm-core` go into the main repo.
* [x] __Move docs from `civicrm-setup.git` into `civicrm-dev-docs.git`__ (*done, see [Dev Guide: Setup Reference](https://docs.civicrm.org/dev/en/latest/framework/setup/)*)
* [ ] __Update Civi-WordPress__:
* [x] (Phase 1) Switch the default from `install/` UI to `civicrm-setup` UI. (*It's currently available opt-in. Switch to opt-out.*)
* [x] (Phase 2) Update `wp-cli/civicrm.php` so that WP-CLI uses `civicrm-setup` API.
* [ ] (Phase 3) Ensure that all tasks (e.g. registering base-pages or permissions) are called in WP-oriented `*.civi-setup.php` plugin.
* [ ] __Update Civi-D8/D9__
* [x] (Phase 1) Update `civicrm.install` to call `civicrm-setup` APIs (related: https://github.com/civicrm/civicrm-drupal-8/pull/37)
* [ ] (Phase 2) Update `civicrm.drush.inc` so that D7/BD use `civicrm-setup` API.
* [ ] __Update Civi-D7/BD__
* [x] (Phase 1) Update `install/index.php` so that D7/BD default to loading the `civicrm-setup` UI. The sysadmin docs may continue pointing admins to `install/index.php`. Optionally, allow one to use a parameter to opt-out and get the old UI.
* [ ] (Phase 2) Update `civicrm.drush.inc` so that D7/BD use `civicrm-setup` API.
* [x] __Update Civi-Joomla__
* [x] (Phase 2) Update to use `civicrm-setup` API
* [x] __Ignore Civi-D6__
* [ ] __Update civibuild__
* [ ] (Phase 2) Switch implementation from `civicrm_install()` to `civicrm_install_cv()` (related WIP: https://github.com/civicrm/civicrm-buildkit/pull/673)
* [ ] (Phase 3) Remove old implementation
* [ ] (Phase ??) Convert `civicrm_make_test_settings_php()` to a `civicrm-setup` plugin. Document an installer-option to initialize test/dev-harness.
* [ ] __Remove all opt-outs. Final pass to double-check/remove all remaining logic from `install/` folder__
* (*This is the last step, after all the others.*)https://lab.civicrm.org/dev/core/-/issues/1634Evaluate if any indexed fields are unused2022-12-04T23:07:09ZeileenEvaluate if any indexed fields are unused
**Proposal** (note this is being updated based on discussion & comments may refer to an earlier version)
1. Remove the following columns from the xml from civicrm_activity
* phone_id
* phone_number
* relationship_id
1. Remove th...
**Proposal** (note this is being updated based on discussion & comments may refer to an earlier version)
1. Remove the following columns from the xml from civicrm_activity
* phone_id
* phone_number
* relationship_id
1. Remove the index from the xml on
* medium_id
* is_deleted
1. During upgrade we drop the above columns, if empty.
**Follow ups to consider**
2. There are other columns in the civicrm_activity table that are case specific - we might consider indexing is_current_revision & original_id only when CiviCase is enabled
3. I've been doing some tests on searches and found that searching is faster if I DROP the contribution_status_id index - it might be interesting to test the activity_type_id index although I suspect it has a much greater cardinality & is more useful
**Impact of the above**
1. Data would not be lost but api fields would no longer access those fields
2. Developers who might be using them outside of core could be impacted - we can mitigate by communicating on the dev list & perhaps putting checks & deprecation notices onto sites with data in the fields for a few months before making any changes.
3. DB size would be reduced. Note that empty fields contribute notably to table size IF they are indexed
4. Dev confusion & efficiency is improved by not having unused stuff in core.
**Background**
Obviously that's not something we should rush into so I'll have to ping the dev list etc
Looking at our civicrm_activity table it appears that each index has a base size - of around a half a gig. From there, the index size increases based on how much data is in the table. So an index on an empty field is around 57% of the size of our largest index.
There are 5 fields that are indexed + empty in our database for the civicrm_activity table (
```
Original id used for CiviCase
Medium id
Phone id
relationship_id
is_deleted
```
Plus - is_current_revision is effectively null
So my first question is are these all used in other databases - e.g when civicase is in use.
I couldn't spot references to phone_id and it feels 'wrong' to me anyway as I think you would want to either link to the contact or have a hard reference. I wonder if some of these fields are quietly obsolete?
It's very unsafe to drop core fields. However, I'm pondering dropping the indexes on these fields
@DaveD I'd appreciate your thoughts....https://lab.civicrm.org/dev/core/-/issues/1726Allow financial types to not have Expense account defined2022-08-26T20:51:52ZJoeMurrayAllow financial types to not have Expense account definedThis is a proposal for discussion and refinement.
Overview
----------------------------------------
Simplify accounting configuration to remove requirement for, and default creation of, widely unused stuff. In particular, don't require ...This is a proposal for discussion and refinement.
Overview
----------------------------------------
Simplify accounting configuration to remove requirement for, and default creation of, widely unused stuff. In particular, don't require Expenses account for every financial type, nor create relations to Expense and Premium accounts by default when creating a financial type.
Example use-case
----------------------------------------
1. Click on **Administer > CiviContribute > Financial Types**.
1. Click **Add Financial Type**.
1. Enter **Name** and click **Save**.
1. In Financial Accounts, there are Banking Fees and Premiums accounts, which is **undesirable**.
1. Click **Accounts** on the new Financial Type row.
1. Beside the 'Expense Account is', click **Delete**, then confirm by clicking **Delete** again.
1. Click on **Contributions > New Contribution**.
1. Select the Financial Type created above that does not have an Expense Account set up for it anymore, fill in **Contributor** and **Total Amount**, and click **Save**.
1. Try to edit the contribution but not in a popup, for example, go to the contact's page, right click on the edit button for the contribution and Copy Link Address, then paste address into a new tab. You'll see "Sorry, due to an error, we are unable to fulfill your request at the moment. You may want to contact your administrator or service provider with more details about what action you were performing when this occurred.
One of parameters (value: ) is not of the type Integer". This is caused by missing Expense account, **even though it is not needed**.
Current behaviour
----------------------------------------
See above.
Proposed behaviour
----------------------------------------
On creation of Financial Type, no Expense or Premiums account relationship would be setup. On editing a contribution (with a line item) with a financial type without an Expense account relationship setup, no error would occur.
Comments
----------------------------------------
The expectation when this was implemented circa 2014 was that payment processors would all soon record banking fees. That hasn't been the case for a variety of reasons.Monish DebMonish Debhttps://lab.civicrm.org/dev/core/-/issues/2467[Meta] XLTS Angular JS longer-term-support2022-12-20T18:24:57Zhomotechsual[Meta] XLTS Angular JS longer-term-support@JoeMurray raised in chat that [AngularJS goes EoL in December](https://chat.civicrm.org/civicrm/pl/cm6jqmcex3n39ekf3zzx4tiree) there is a company [XLTS](https://xlts.dev/angularjs) providing extended LTS support through 2026. I've send ...@JoeMurray raised in chat that [AngularJS goes EoL in December](https://chat.civicrm.org/civicrm/pl/cm6jqmcex3n39ekf3zzx4tiree) there is a company [XLTS](https://xlts.dev/angularjs) providing extended LTS support through 2026. I've send them an enquiry asking how much they are likely to look to charge given the open-source nature of CiviCRM to see if, perhaps, they'd be willing to "donate" the XLTS to us.
We lose nothing by asking!JoeMurrayJoeMurrayhttps://lab.civicrm.org/dev/core/-/issues/2795PHP8: Use of error-suppression operator (@) behaves differently in error_hand...2023-12-12T19:51:20ZDaveDPHP8: Use of error-suppression operator (@) behaves differently in error_handler functions in php8Parking what's left in https://github.com/civicrm/civicrm-core/pull/21064 here for now since I've cleared out a bunch and am taking a break from it. The gist is that in php8 the error suppression operator (e.g. `@fopen('/nonexistent/file...Parking what's left in https://github.com/civicrm/civicrm-core/pull/21064 here for now since I've cleared out a bunch and am taking a break from it. The gist is that in php8 the error suppression operator (e.g. `@fopen('/nonexistent/file.txt', 'r')` works differently with respect to error_handler functions. In a stock install as of right now you won't notice any difference, just there will be more errors depending on what 3rd party/custom error_handlers you're using and may behave differently on different CMSs in future depending on how their error-handling may change.
### smarty default modifier
The smarty modifier `|default` is a tricky one because its whole purpose is to deal with the situation where the var might be missing, but the way smarty compiles it is something like `<?php $this->assign('to', ((is_array($_tmp=@$this->_tpl_vars['to'])) ? $this->_run_mod_handler('default', true, $_tmp, '_high') : smarty_modifier_default($_tmp, '_high'))); ?>` which uses the @ to try to silence the error. In addition to [templates/CRM/Core/DatePickerRange.tpl and templates/CRM/Core/DatePickerRangeWrapper.tpl](https://github.com/civicrm/civicrm-core/commit/f671f500b658aa772fedc682dbb320cf456bc904), the below are all instances of this. They could all be rewritten to not use the modifier, but the modifier itself is part of smarty so can't easily be prevented from being used in future.
* CRM_Core_InvokeTest::testInvokeDashboardForNonAdmin
* CRM/Contact/Page/DashboardDashlet.tpl - $communityMessages
* CRM_Core_SessionTest::testSetStatusWithTextOnly
* CRM/common/info.tpl
* CRM_Core_SessionTest::testSetStatusWithTitleOnly
* CRM/common/info.tpl
* CRM_Core_SessionTest::testSetStatusWithBoth
* CRM/common/info.tpl
### opendir
```
CRM_Utils_File::cleanDir
if ($dh = @opendir($target)) {
```
### unlink and mkdir
Various calls to `@unlink` and `@mkdir`
### httpclient
```
CRM_Utils_HttpClientTest::testFetchHttp_badOutFile
Exception: fopen(/ba/d/path/too/utput): Failed to open stream: No such file or directory
...\tests\phpunit\CiviTest\CiviUnitTestCase.php:253
...\CRM\Utils\HttpClient.php:79 <---- see here
...\tests\phpunit\CRM\Utils\HttpClientTest.php:77
...\tests\phpunit\CiviTest\CiviUnitTestCase.php:267
...\phpunit8:671
```
### angular changeset
Aside: The code in question here suggests phpQuery has its own debug setting which maybe could be turned on automatically in future if you have civi debugging enabled:
`$return = phpQuery::$debug === 2 ? this->document->loadHTML($markup) : @$this->document->loadHTML($markup);`
```
Civi\Angular\ChangeSetTest::testInsertAfter
Exception: DOMDocument::loadHTML(): htmlParseEntityRef: no name in Entity, line: 2
...\tests\phpunit\CiviTest\CiviUnitTestCase.php:253
...\vendor\electrolinux\phpquery\phpQuery\phpQuery\DOMDocumentWrapper.php:200
...\vendor\electrolinux\phpquery\phpQuery\phpQuery\DOMDocumentWrapper.php:542
...\vendor\electrolinux\phpquery\phpQuery\phpQuery\DOMDocumentWrapper.php:505
...\vendor\electrolinux\phpquery\phpQuery\phpQuery\DOMDocumentWrapper.php:469
...\vendor\electrolinux\phpquery\phpQuery\phpQuery\phpQueryObject.php:2066
...\vendor\electrolinux\phpquery\phpQuery\phpQuery\phpQueryObject.php:2010
...\tests\phpunit\Civi\Angular\ChangeSetTest.php:28
...\Civi\Angular\ChangeSet.php:59
...\Civi\Angular\ChangeSet.php:19
...\tests\phpunit\Civi\Angular\ChangeSetTest.php:39
...\tests\phpunit\CiviTest\CiviUnitTestCase.php:267
...\phpunit8:671
```
* not ok 103 - Error: Civi\Angular\PartialSyntaxTest::testConsistencyExamples with data set 6 `('<div ng-if="a && b"></div>', '<div ng-if="a && b"></div>')`
* not ok 104 - Error: Civi\Angular\PartialSyntaxTest::testAllPartials
* These are the same issue as the changeset tests.
### testing of deprecations
These deliberately test deprecations
* not ok 223 - Error: Civi\Payment\PropertyBagTest::testSetContactIDLegacyWay
* not ok 227 - Error: Civi\Payment\PropertyBagTest::testSetCustomProp
### testGetfieldsHasTitle
```
api_v3_SyntaxConformanceTest::testGetfieldsHasTitle with data set 32 ('Entity')
Failure in api call for Entity getactions: include_once(api/v3/batch_create.php): failed to open stream: No such file or directory
0 ...\api\v3\Entity.php(18): CiviUnitTestCase->letssee(2, 'include_once(ap...', '\path\to\...', 18, Array)
1 ...\api\v3\Entity.php(18): include_once()
2 ...\api\v3\utils.php(2461): _civicrm_api3_entity_deprecation(Array)
3 ...\api\v3\utils.php(251): _civicrm_api3_deprecation_check('Entity', Array)
4 ...\Civi\API\Provider\ReflectionProvider.php(108): civicrm_api3_create_success(Array, Array, 'Entity', 'getactions')
5 ...\Civi\API\Kernel.php(149): Civi\API\Provider\ReflectionProvider->invoke(Array)
6 ...\Civi\API\Kernel.php(81): Civi\API\Kernel->runRequest(Array)
7 ...\api\api.php(22): Civi\API\Kernel->runSafe('Entity', 'getactions', Array)
8 ...\Civi\Test\Api3TestTrait.php(292): civicrm_api('Entity', 'getactions', Array)
9 ...\Civi\Test\Api3TestTrait.php(173): CiviUnitTestCase->civicrm_api('Entity', 'getactions', Array)
10 ...\tests\phpunit\api\v3\SyntaxConformanceTest.php(1662): CiviUnitTestCase->callAPISuccess('Entity', 'getactions', Array)
11 phar://.../phpunit8/phpunit/Framework/TestCase.php(1213): api_v3_SyntaxConformanceTest->testGetfieldsHasTitle('Entity')
12 ...\tests\phpunit\CiviTest\CiviUnitTestCase.php(267): PHPUnit\Framework\TestCase->runTest()
13 phar://.../phpunit8/phpunit/Framework/TestCase.php(889): CiviUnitTestCase->runTest()
14 phar://.../phpunit8/phpunit/Framework/TestResult.php(577): PHPUnit\Framework\TestCase->runBare()
15 phar://.../phpunit8/phpunit/Framework/TestCase.php(663): PHPUnit\Framework\TestResult->run(Object(api_v3_SyntaxConformanceTest))
16 phar://.../phpunit8/phpunit/Framework/TestSuite.php(481): PHPUnit\Framework\TestCase->run(Object(PHPUnit\Framework\TestResult))
17 phar://.../phpunit8/phpunit/Framework/TestSuite.php(481): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
18 phar://.../phpunit8/phpunit/TextUI/TestRunner.php(462): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
19 phar://.../phpunit8/phpunit/TextUI/Command.php(129): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framework\TestSuite), Array, Array, true)
20 phar://.../phpunit8/phpunit/TextUI/Command.php(101): PHPUnit\TextUI\Command->run(Array, true)
21 ...\phpunit8(671): PHPUnit\TextUI\Command::main()
22 {main}
Failed asserting that a integer is empty.
...\Civi\Test\Api3TestTrait.php:110
...\Civi\Test\Api3TestTrait.php:174
...\tests\phpunit\api\v3\SyntaxConformanceTest.php:1662
...\tests\phpunit\CiviTest\CiviUnitTestCase.php:267
...\phpunit8:671
```
### mailing testConcurrency
This doesn't run properly on windows even without the error_handler, and I don't feel like taking the time right now to deal with that:
not ok 1161 - Error: api_v3_JobProcessMailingTest::testConcurrency with data set #0 (array(20, 3, 10, 4, 1), array(2, 1), 4)https://lab.civicrm.org/dev/core/-/issues/2811Menu Angular error after upgrade2023-01-08T01:26:08ZStoobMenu Angular error after upgradeI do a fair amount of upgrades, and anecdotally after about a _half_ of recent upgrades, the CiviCRM menu fails to load. For half of those, a simple reload/refresh of the browser resolves the issue. For the other half, the civicrm/menu...I do a fair amount of upgrades, and anecdotally after about a _half_ of recent upgrades, the CiviCRM menu fails to load. For half of those, a simple reload/refresh of the browser resolves the issue. For the other half, the civicrm/menu/rebuild cache (or drush cc) must be cleared in order for the menu to appear. It is usually an Angular issue, seen attached.
Two questions, if these steps are required with such frequency can the upgrade process be improved by either:
1. clearing the cache automatically when the upgrade is finished
2. providing instructions and/or link on the upgrade screen to clear cache
![menu-load](/uploads/70b9bc8bcfbae067b5169d330bd16545/menu-load.png)
![upg](/uploads/19aa40afb6bda323d97cafe3c3cec5b3/upg.png)https://lab.civicrm.org/dev/core/-/issues/2945Make CRM_Core_Smarty::singleton() use Civi::statics instead of a static var2023-10-13T05:11:14ZDaveDMake CRM_Core_Smarty::singleton() use Civi::statics instead of a static varDuring unit tests, static vars can be problematic because they don't get reset between tests. There's a point within smarty where if a test fails at that point then its secure mode doesn't get reset. It's come up a couple times and then ...During unit tests, static vars can be problematic because they don't get reset between tests. There's a point within smarty where if a test fails at that point then its secure mode doesn't get reset. It's come up a couple times and then all the tests that use smarty on certain forms fail after it. It's not easy to track down if you haven't seen it before.
There's an argument civi code should be smarty secure-mode-compliant instead, but it isn't at the moment.https://lab.civicrm.org/dev/core/-/issues/3740Set default lock level to READ COMMITTED2023-10-20T07:57:25ZJoeMurraySet default lock level to READ COMMITTEDDrupal recently changed the default lock level slightly down from REPEATABLE READ to READ COMMITTED, which has the effect of avoiding some db locks. See https://www.drupal.org/node/3269885 We have seen over recent years some issues with ...Drupal recently changed the default lock level slightly down from REPEATABLE READ to READ COMMITTED, which has the effect of avoiding some db locks. See https://www.drupal.org/node/3269885 We have seen over recent years some issues with deadlocks, primarily on rebuilding group contact cache when there are hierarchies of smart groups. Should CiviCRM be more explicit about what isolation level we expect in order to get more repeatable results, and should we follow Drupal in its move to READ COMMITTED? I tend to think so.
See responses in MM chat from @demeritcowboy
https://chat.civicrm.org/civicrm/pl/gnmhfdxes7rgb8k7ppoaezo45ahttps://lab.civicrm.org/dev/core/-/issues/3833PHP 8.2 Dynamic Properties are deprecated2024-03-22T12:22:59ZBradley TaylorPHP 8.2 Dynamic Properties are deprecatedSee https://wiki.php.net/rfc/deprecate_dynamic_properties.
I see this as a big one, which could require a significant number of fixes for CiviCRM. The RFC has a good example showing what has changed:
```
class User {
public $name;
...See https://wiki.php.net/rfc/deprecate_dynamic_properties.
I see this as a big one, which could require a significant number of fixes for CiviCRM. The RFC has a good example showing what has changed:
```
class User {
public $name;
}
$user = new User;
// Assigns declared property User::$name.
$user->name = "foo";
// Oops, a typo:
$user->nane = "foo";
// PHP <= 8.1: Silently creates dynamic $user->nane property.
// PHP 8.2: Raises deprecation warning, still creates dynamic property.
// PHP 9.0: Throws Error exception.
```
There are many uses of dynamic properties in CiviCRM core which fall into multiple categories:
1. Known undeclared properties. For example `CRM_Activity_Form_Task_Batch` repeatedly references `$_fields` which should just be defined.
2. Dynamic properties. The main example of this is `CRM_Core_DAO`. PHP allows for this use-case by either extending `stdClass` or using the new ` #[AllowDynamicProperties]` attribute. Where possible we should prefer extending `stdClass` which is likely to have better long-term support (i.e. into the PHP 9.x series)
3. Typo's etc. Cases where the property is defined, but later referenced with a different name. These are easy; we just fix the typo (whilst ensuring we don't break any backwards compatiability for plugins using the wrong spelling)
I suggest CiviCRM developers should start to be made aware of this now, to avoid making the problem worse. Incrementally starting to fix the issue, a class or two at a time, seems like a good idea.
In many cases it's not clear why dynamic properties (or declared properties for that matter) are being used instead of a standard variable. This might be a good chance to start annotating properties as either:
- `@api`; designed to be used within hooks, on singletons etc by CiviCRM extensions
- `@internal`; not designed to be used by extensions, and liable to be removed without deprecation.https://lab.civicrm.org/dev/core/-/issues/4349Migrate "Edit Profile" popup to SearchKit/FormBuilder, kill BackBone2023-06-09T09:14:03ZcolemanwMigrate "Edit Profile" popup to SearchKit/FormBuilder, kill BackBoneCiviCRM includes an entire javascript framework stack, Backbone + Marionette, and only uses it to do one thing: the "Edit Profile" popup.
It wouldn't be quite the same, but I think we could make something equivalent *enough* using Search...CiviCRM includes an entire javascript framework stack, Backbone + Marionette, and only uses it to do one thing: the "Edit Profile" popup.
It wouldn't be quite the same, but I think we could make something equivalent *enough* using SearchKit and Afform and kill off Backbone once and for all.https://lab.civicrm.org/dev/core/-/issues/4433E2E_Core_PathUrlTest::testGetUrl_WpAdmin() fails because CiviCRM routing is c...2023-07-24T20:52:34ZtottenE2E_Core_PathUrlTest::testGetUrl_WpAdmin() fails because CiviCRM routing is confusingThe gist of the test: it calls `cv url civicrm/contribute?reset=1` and asserts that the URL will open in the WordPress backend UI (aka `/wp-admin/`).
([Full source](https://github.com/civicrm/civicrm-core/blob/8e0dabd20bebe55d5dd725f301...The gist of the test: it calls `cv url civicrm/contribute?reset=1` and asserts that the URL will open in the WordPress backend UI (aka `/wp-admin/`).
([Full source](https://github.com/civicrm/civicrm-core/blob/8e0dabd20bebe55d5dd725f3015c41019cb8dbed/tests/phpunit/E2E/Core/PathUrlTest.php#L94-L117))
The test is failing. There are currently two patches:
* [25476](https://github.com/civicrm/civicrm-core/pull/25476): Automatically choose frontend/backend based on the route metadata
* [26772](https://github.com/civicrm/civicrm-core/pull/26772): Change the test to specifically request backend
Both have issues. So here's a deep-dive into the context.
Background
----------
* CiviCRM runs on Drupal/Backdrop and WordPress/Joomla.
* On WordPress/Joomla, the "frontend" and "backend" are different sub-applications (e.g. `/` vs `/wp-admin/`). The applications have very different URL structures. To make a hyperlink, you first decide which sub-application to target -- and then pick a page within it.
* On Drupal/Backdrop, it's one application, and all URLs have similar structure. To make a hyperlink, you simply identify the page.
* (*Drupal/Backdrop UX can sometimes distinguish frontend/backend -- but it's a visual choice based on local configuration. It's not a structural part of the URL.*)
* CiviCRM's routing system integrates into every UF/CMS, but (internally) it is closer to Drupal's. There is one `civicrm_menu` with all frontend+backend pages.
* On Drupal/Backdrop, CiviCRM's routes pass directly to CMS routes.
* On WordPress/Joomla, CiviCRM's routing integrates with both subapplications (ie "frontend" and "backend").
* CiviCRM stores a flag `bool is_public` for each route.
* CiviCRM includes routes which can be classified as:
* Purely backend (ex: `civicrm/dashboard`)
* Purely frontend (ex: `civicrm/event/register`)
* Purely web-service (ex: `civicrm/payment/ipn`)
* Multi-homed
* Ex: `civicrm/profile/view` has use-cases for frontend and backend
* Ex: `civicrm/ajax/api4/%` has use-cases for frontend, backend, and web-service
* CiviCRM has a function `CRM_Utils_System::url()`
* At first glance, it resembles Drupal's `\url()`. You typically just give the page (e.g. `url('civicrm/foo/bar')`).
* Over time, several additional parameters were added -- notably, the 6th parameter `bool $frontend` and the 7th parameter `bool $forceBackend`.
Problems
--------
* The status-quo invites bugs between CMS's.
* A developer working on Drupal will have trouble recognizing the importance of the 6th and 7th parameters. (*Those params do nothing on Drupal - and they're buried at the end of method.*)
* The status-quo invites bugs between interactive and automatic processes.
* A developer defines a custom token and tests it interactively; then at runtime, the token is sent by an automatic process. But the interactive/automatic split is orthogonal to frontend/backend split. Sometimes, interactive/automatic agree with each other (*both frontend or both backend*), and sometimes they disagree (*one frontend, one backend*).
* Overall, what tends to happen is:
* Developer writes a call like `CRM_Utils_System::url('civicrm/foo/bar')`, and it looks pretty.
* They test, and it works beautifully on their system.
* They publish, and it fails on other systems.
* Someone writes a patch to add 5 more parameters (`url('civicrm/foo/bar', '', FALSE, NULL, TRUE, TRUE)`). And then it's OK.
* The DX is awkward and invites bugs (in core and contrib) -- but it becomes more annoying for `cv` UX.
* If you need a 5th/6th param in PHP code, then you'll eventually figure it out and commit the update to your codebase. Then you forget about it.
* `cv` has some commands to facilitate manual testing and E2E testing (ie `cv url`, `cv http`, `cv open`). These are things that you improvise. Mismatched URLs can be annoying anytime you use these subcommands.
What to do
----------
There are two PRs to fix the test, and honestly - I don't really like either.
* [26772](https://github.com/civicrm/civicrm-core/pull/26772) makes the red mark go away, but it leaves the underlying issue (poor DX for PHP and poor UX for cv).
* [25476](https://github.com/civicrm/civicrm-core/pull/25476) aims to fix the underlying issue, but it's probably too facile. The current metadata can distinguish "purely frontend" pages from "purely backend" pages, but it cannot recognize "purely web-service" or "multi-homed". It probably messes-up some scenarios for those.
* Fixing this probably requires some more aggressive transition in the contract.
* Ex: Improve the routing metadata so that we can distinguish web-service routes and multi-home routes.
* Ex: Define a different class or function for generating URLs (*with a more usable signature*).
* Another option is to leave `civicrm-core` as-is -- and only update `cv`.
* Ex: Give `cv` the mechanism to resolve a frontend/backend based on metadata.
* Ex: Change `cv` to complain if you don't a specify frontend/backend flag.
* Either way, it feels like a bit of a wasted opportunity to only patch `cv` when we know that other users of `CRM_Utils_System::url()` get confused about the frontend/backend flags.https://lab.civicrm.org/dev/core/-/issues/4581Define re-usable idiom for deferrable upgrade steps2023-09-19T13:05:13ZtottenDefine re-usable idiom for deferrable upgrade stepsOverview
----------------------------------------
In managing upgrade-steps, there is some tension between:
* Making the upgrades automatic for typical sysadmins
* Allowing very large deployments to schedule/manage expensive upgrade-st...Overview
----------------------------------------
In managing upgrade-steps, there is some tension between:
* Making the upgrades automatic for typical sysadmins
* Allowing very large deployments to schedule/manage expensive upgrade-steps
This proposes to balance those expectations by defining a _deferrable upgrade task_. Sysadmins can choose to approach them a few ways:
* "Just run the task like every other upgrade step" (*e.g. default; for average and small systems*)
* "Let me decide when to run it... but please don't make me figure out obscure incantations" (*for larger systems*)
* "Let me find a highly-optimized/bespoke way to accomplish the change" (*for extra-large systems with a team of admins*)
Example use-case
----------------------------------------
1. Take a table (like `civicrm_contact`, `civicrm_activity`, `civicrm_mailing_event_queue`) which can be very large on some deployments.
1. Define a schema change (e.g. add a column and an index)
1. Executing this change be quite slow.
1. In the long-term, all systems need the schema-change. If people skip it, then it will lead to problems/confusion/bug-reports/etc.
1. In the near-future, the schema change is not quite mandatory. (The new column isn't actively used; or its usages are limited+guarded.)
A sketch
----------------------------------------
* Designate a place to store a list of deferred upgrade steps (e.g. `civicrm_deferred_upgrade` or `civicrm_queue_item`)
* In the `CRM/Upgrade/` system, add some helpers to conditionally execute or defer a step. Ex:
```php
$this->addDeferrableTask(ts('Update CiviMail tracking'), 'doCiviMailQueueConversion');
```
```php
function addDeferrableTask($title, $funcName, $args) {
if (deferred upgrades enabled) {
// Add $title, $action, $args to a persistent TODO list
}
else {
$this->addTask($title, $funcName, $args);
}
}
```
* Add a system-status-check and post-upgrade message to warn if there are any of these things that need to run
* Add an API/job/command/UI to list/execute/skip/delete deferred stepshttps://lab.civicrm.org/dev/core/-/issues/4699MessageTemplate - Graduate new editing UI2023-10-16T09:41:00ZtottenMessageTemplate - Graduate new editing UIRe: @eileen's comment on https://github.com/civicrm/civicrm-core/pull/24981#issuecomment-1753901505
> as an aside - I was thinking about the fact there are 2 parts to the message admin - this search display & the edit form (well those a...Re: @eileen's comment on https://github.com/civicrm/civicrm-core/pull/24981#issuecomment-1753901505
> as an aside - I was thinking about the fact there are 2 parts to the message admin - this search display & the edit form (well those are the major parts).
>
> I feel like the edit form is mature enough to remove the quick form one - but where would it sit?
My first impulse was skeptical, but now I kinda see it. Let me try to talk it through:
* When we implemented the new editor (for translatable system-workflow messages), we put it in an extension (`message_admin`) for fear that it wouldn't be at par with the existing editor.
* The "Message Templates" are two things -- "user-defined templates" and "system-workflow messages". The suitability depends on how strongly you make that distinction.
* The attitude can be: "_They're both `MessageTemplate`, so they're the same thing, so they should use the same editor_" ... In that case, no, the current `message_admin` editor is not ready to handle "user-defined templates".
* The attitude can be "_They're really different things, and we should split them apart_" .... In that case, yes, I think we could basically elevate the `message_admin` editor-screen for system-workflow messages on all sites.
* In brainstorming with Coleman for #4454, I quite liked the idea of providing separate nav-links for those screens. (Add a separate link for "Admin > Communications > Workflow Messages".)
There are a few differences between the screens:
* Editing widget
* The old editor uses the `ckeditor` widget. User-defined templates lean more on prose and layout. Using a rich-text widget is more agreeable.
* The new editor uses the `monaco` widget. Workflow-message templates lean more on logic and data. Using a structured widget is more agreeable.
* Missing options
* The old editor has some "PDF" options
* The old editor has an option to upload a document (instead of editing in browser).
* Extra options
* There are various options+buttons that appear in the new workflow-message editor -- but haven't been thought-through for the user-defined stuff - e.g. "Original", "Draft", "Locale", "Activate"
IMHO, this would be the shortest path to elevating/blessing the new editor for all sites:
1. Conceptually, accept "User-defined templates" and "System-workflow messages" as different things (with different screens).
2. Setup separate links/titles for the pages
3. Examine (and possibly port) the missing options (esp PDF dropdown).
4. Move the editor to core.https://lab.civicrm.org/dev/core/-/issues/4952PHP 8.3 Support2024-01-31T22:32:21ZJoeMurrayPHP 8.3 SupportThis is an epic for PHP 8.3 support.
PHP 8.3 was released 2023-11, has active support till 2024-12-8, and security support till 2025-12-08.
Some extra motivation to support 8.3 sooner rather that later is the significant performance im...This is an epic for PHP 8.3 support.
PHP 8.3 was released 2023-11, has active support till 2024-12-8, and security support till 2025-12-08.
Some extra motivation to support 8.3 sooner rather that later is the significant performance improvements it has, up to 55% for D10 over 8.1: https://kinsta.com/blog/php-benchmarks/
Reference: [https://www.php.net/manual/en/migration83.incompatible.php](https://www.php.net/manual/en/migration83.incompatible.php)
See also [thttps://php.watch/versions/8.3](https://php.watch/versions/8.3) and [https://www.php.net/releases/8.3/en.php](https://www.php.net/releases/8.3/en.php).
Here are the breaking changes listed here for reference. Almost all seem like they would be difficult to search for and identify in the codebase. Whack-a-mole from running on edge test servers might be a helpful approach.
- [ ] Uses of traits with static properties: Uses of traits with static properties will now redeclare static properties inherited from the parent class. This will create a separate static property storage for the current class. This is analogous to adding the static property to the class directly without traits.
- [ ] Assigning a negative index to an empty array: Assigning a negative index $n to an empty array will now make sure that the next index is $n+1 instead of 0.
- [ ] Class constant visibility variance check: Class constant visibility variance is now correctly checked when inherited from interfaces.
- [ ] WeakMap entries whose key maps to itself: WeakMap entries whose key maps to itself (possibly transitively) may now be removed during cycle collection if the key is not reachable except by iterating over the WeakMap (reachability via iteration is considered weak). Previously, such entries would never be automatically removed.
- [ ] Date: The DateTime extension has introduced Date extension specific exceptions and errors under the DateError and DateException hierarchies, instead of warnings and generic exceptions. This improves error handling, at the expense of having to check for errors and exceptions.
- [ ] DOM: Calling DOMChildNode::after(), DOMChildNode::before(), DOMChildNode::replaceWith() on a node that has no parent is now a no-op instead of a hierarchy exception, which is the behaviour demanded by the DOM specification.
Using the DOMParentNode and DOMChildNode methods without a document now works instead of throwing a DOM_HIERARCHY_REQUEST_ERR DOMException. This is in line with the behaviour demanded by the DOM specification.
Calling DOMDocument::createAttributeNS() without specifying a prefix would incorrectly create a default namespace, placing the element inside the namespace instead of the attribute. This bug is now fixed.
DOMDocument::createAttributeNS() would previously incorrectly throw a DOM_NAMESPACE_ERRNAMESPACE_ERR DOMException when the prefix was already used for a different URI. It now correctly chooses a different prefix when there's a prefix name conflict.
New methods and properties were added to some DOM classes. If a userland class inherits from these classes and declare a method or property with the same name, the declarations must be compatible. Otherwise, a typical compile error about incompatible declarations will be thrown. See the list of new features and new functions for a list of the newly implemented methods and properties.
- [x] FFI: C functions that have a return type of void now return null instead of returning the following object object(FFI\CData:void) { }
- [ ] Opcache: The opcache.consistency_checks INI directive was removed. This feature was broken with the tracing JIT, as well as with inheritance cache, and has been disabled without a way to enable it since PHP 8.1.18 and PHP 8.2.5. Both the tracing JIT and inheritance cache may modify shm after the script has been persisted by invalidating its checksum. The attempted fix skipped over the modifiable pointers but was rejected due to complexity. For this reason, it was decided to remove the feature instead.
- [ ] Phar: The type of Phar class constants are now declared.
- [ ] Standard: The range() function has had various changes:
A TypeError is now thrown when passing objects, resources, or arrays as the boundary inputs.
A more descriptive ValueError is thrown when passing 0 for $step.
A ValueError is now thrown when using a negative $step for increasing ranges.
If $step is a float that can be interpreted as an int, it is now done so.
A ValueError is now thrown if any argument is infinity or NAN.
An E_WARNING is now emitted if $start or $end is the empty string. The value continues to be cast to the value 0.
An E_WARNING is now emitted if $start or $end has more than one byte, only if it is a non-numeric string.
An E_WARNING is now emitted if $start or $end is cast to an integer because the other boundary input is a number. (e.g. range(5, 'z');).
An E_WARNING is now emitted if $step is a float when trying to generate a range of characters, except if both boundary inputs are numeric strings (e.g. range('5', '9', 0.5); does not produce a warning).
range() now produce a list of characters if one of the boundary inputs is a string digit instead of casting the other input to int (e.g. range('9', 'A');).
```
<?php
range('9', 'A'); // ["9", ":", ";", "<", "=", ">", "?", "@", "A"], as of PHP 8.3.0
range('9', 'A'); // [9, 8, 7, 6, 5, 4, 3, 2, 1, 0], prior to PHP 8.3.0
?>
```
The file() flags error check now catches all invalid flags. Notably FILE_APPEND was previously silently accepted.
- [ ] SNMP: The type of SNMP class constants are now declared.https://lab.civicrm.org/dev/core/-/issues/4999Imagine a world without CodeGen2024-03-05T04:32:51ZcolemanwImagine a world without CodeGenCurrently we use `CRM_Core_CodeGen` to take our schema/xml files and generate DAO.php, install.sql and uninstall.sql files, which have to be periodically regenerated. This is a minor inconvenience for a core developer, a potential gotcha...Currently we use `CRM_Core_CodeGen` to take our schema/xml files and generate DAO.php, install.sql and uninstall.sql files, which have to be periodically regenerated. This is a minor inconvenience for a core developer, a potential gotcha for an extension developer, and a major coordination difficulty across *all* extensions in `universe` (whenever any aspect of the generated code needs to change).
But what if we didn't have to generate those files? What if we could read schema information directly from the xml (or potentially a different source).
Current Structure:
-----
**Key:** ✍️ Handwritten file | 📠 Generated file
| File | Purpose | In Core | In Extensions |
| --- | --- | --- | --- |
| ✍️ `schema/xml` | Canonical declaration of entity + all metadata | Run `setup.sh -g` | Run `civix generate:entity-boilerplate` |
| 📠 Install sql | Add schema tables | `civicrm.mysql` | `auto_install.sql` |
| 📠 Uninstall sql | Drop schema tables | `civicrm_drop.mysql` | `auto_uninstall.sql` |
| 📠 Entity.php | Declare entity's existence | `AllCoreTables.data.php` | `*.entityType.php` + `entity-types-php` mixin |
| 📠 `CRM/Core/I18n/SchemaStructure.php` | Lists localizable table columns | Seems kinda redundant with other metadata? | Doesn't exist |
| ✍️ `CRM_Core_DAO` | Base class for all generated DAOs | All DAOs extend this class | Extension DAOs also extend core class (makes change-management across `universe` difficult) |
| 📠 `CRM_*_DAO_*` | Generated from the xml file | Must be generated | Must be generated |
The generated DAO file (including stuff it inherits from `CRM_Core_DAO`) serves a variety of purposes:
1. OO class that allows a database table to be used like a php object, e.g.
```
$contact = new CRM_Contact_DAO_Contact();
$contact->first_name = 'Bob';
$contact->save();
```
This is a neat idea and probably ahead of its time, but is basically deprecated now, mostly because, in a relational database, having access to only one table at a time isn't very useful. If it weren't for legacy core & extension code still using this pattern, we could drop it in favor of more robust tools like APIv4.
2. Static methods like `fields()` and `indices()` which return the data from the xml file in php format.
3. Localizes strings, because CodeGen wraps titles and labels in `ts()`.
4. A bunch of other random static methods (e.g. `disableFullGroupByMode()`) which seem like they'd be better-placed in a separate utility class.
New Structure
----------
If we no longer want to generate files and just read from a canonical source, then the main question is, "what should be the canonical source of entity metadata?"
1. **Stick with XML:** Keep the existing xml files but delete the generated stuff. Parse the xml at runtime to get that data.
- Pro: It's already there, no rewrites needed.
- Con: Poor DX (developers don't generally enjoy writing XML files).
- Con: It's very slow (the slowest by far of all the options) so heavy caching would be needed.
- Con: Without generating php files, another method of i18n string extraction would be needed ([such as this](https://github.com/civicrm/civistrings/pull/17)).
2. **DAO Files:** Delete the `schema/xml` files and run everything from the generated DAO files, which going forward will be hand-edited instead of regenerated.
- Pro: DAO files are already there.
- Con: Also poor DX (the boilerplate in those files would not be fun to write/edit by hand).
3. **Somewhere Else:** Move schema info to e.g. json files or better-structured PHP files.
- Pro: DX and performance could be optimized.
- Con: XML files must be rewritten (could be scripted).
- Con: Migration management for core and extensions.
Supporting Dynamic/Virtual Entities
--------
It's also worth keeping in mind that there are now several types of entities that are dynamic & share a DAO:
- Multi-record custom fields
- ECK entities
- SearchKit materialized displays
The DAO structure doesn't cope with this very well, as the assumption has always been 1-1-1 between table, entity & DAO class. But while we're restructuring things let's avoid adding more code that makes this assumption. An ideal DAO from the POV of virtual entities would be an object that takes entity name in its constructor & initializes itself with the appropriately corresponding tablename, fields, and other metadata.