CiviCRM Core issueshttps://lab.civicrm.org/dev/core/-/issues2022-12-07T02:47:08Zhttps://lab.civicrm.org/dev/core/-/issues/1532Migrate case activity revisions to trigger-based logging2022-12-07T02:47:08ZDaveDMigrate case activity revisions to trigger-based loggingThe old description below is currently still true but this is now about migrating to trigger-based logging.
<details><summary>click to see old boring description</summary>
This is an old topic, but I'm making this ticket as a place to ...The old description below is currently still true but this is now about migrating to trigger-based logging.
<details><summary>click to see old boring description</summary>
This is an old topic, but I'm making this ticket as a place to put some notes related to https://github.com/civicrm/civicrm-core/pull/16283
Background:
Before detailed/trigger-based logging was introduced, civicase always had a mechanism to keep revisions of every activity. Over time a couple things happened that led to it no longer working in the UI, although it still worked if you used the api to update case activities. The revisions mechanism isn't very efficient, but it's still there.
When I started looking into it about a year ago, there didn't seem to be much interest, so I only took a quick look and what was odd was that it seemed like it SHOULD still be working. Then when the PR was posted I started fiddling and was surprised/confused by the fact that a unit test calling the form code, where I was trying to show that it should be failing, was working, and I could see the test was creating revisions! But it's clearly not working when done through the UI.
It turns out that whether or not you have any custom fields defined, in CRM_Case_Form_Activity::postProcess() the param 'hidden_custom' always comes in as '1' when run in real life, and so a line that had been put in a few years ago (and later further quickie-patched) gets run that causes the 'id' param to get set to the current activity id regardless of whether you're making a revision or not, so it never makes the new revision and just updates that activity. [This line](https://github.com/civicrm/civicrm-core/blob/5.21.0/CRM/Case/Form/Activity.php#L431): `$params = array_merge($params, $oldActivity);`.
There's also, now with the above PR, some redundant stuff that causes at least one problem and I'm still checking to see if there's more. If you have regular attachments (as opposed to custom fields of type file) it ends up calling [CRM_Core_BAO_File::copyEntityFile](https://github.com/civicrm/civicrm-core/blob/5.21.0/CRM/Case/Form/Activity.php#L544) twice because of the new api call which also deals with it, which leads to duplicate db key errors.
Additionally, there are some lines that, at least as far as I can see, do nothing except prepopulate a static cache that doesn't seem to need prepopulating, and the local variable is never used: https://github.com/civicrm/civicrm-core/blob/5.21.0/CRM/Case/Form/Activity.php#L450-L456. They don't seem to cause problems, but it makes me wonder what I'm missing.
The unit test is still WIP but is at the link below, along with some other fiddlings but ignore those. Given that a difficulty with revisions is custom fields and data like attachments I'd like to expand the test to include those, otherwise it might lead to a patch-break-patch-break-patch-break situation again. https://github.com/civicrm/civicrm-core/compare/master...demeritcowboy:16283-testing#diff-d2708831d5c3b219fce5d221bc3b17c6
</details>https://lab.civicrm.org/dev/core/-/issues/1516Discussion ticket for handling multivalued tokens in pdfs/emails2023-09-25T14:58:11ZDaveDDiscussion ticket for handling multivalued tokens in pdfs/emailsIn particular related to:
* https://github.com/civicrm/civicrm-core/pull/12012
* and its rebased version https://github.com/civicrm/civicrm-core/pull/16200
* https://github.com/civicrm/civicrm-core/pull/16208
There are a number of way...In particular related to:
* https://github.com/civicrm/civicrm-core/pull/12012
* and its rebased version https://github.com/civicrm/civicrm-core/pull/16200
* https://github.com/civicrm/civicrm-core/pull/16208
There are a number of ways to deal with multivalued fields when you need to produce scalar output based on the field.
For example:
* Take the "first" one.
* This has the disadvantage that "first" doesn't really mean anything, especially if the order is not guaranteed to be the same each time.
* Take the "last" one.
* Ditto.
* Combine them separated by commas.
* This has the disadvantage that it may not make any sense or might look silly. Consider something like the "With Contact" on an activity. Suppose there are 3. Suppose you want to include the primary phone of the contact. Your pdf might look like this. It's not terrible, but now think about adding in address too.
* With Contact: John Smith, Mary Brown, Indiana Jones
* Phone: 555-123-2222, , 555-383-8383
* And it's not clear to me that it can guarantee that the phone values are in the same order as the names when the tokens are separate.
* Do as in https://github.com/civicrm/civicrm-core/pull/12012, where you give the template designer the option to specify a numeric suffix on the token, e.g. {activity.target_2_display_name}. The way it's implemented there has some limitations:
* The order is still arbitrary - what does "2" actually mean?
* The template designer doesn't know max(N), so they can't easily do something like concatenate all of them if they want to.
* Provide some shorthand tokens for some common uses, e.g.
* {activity.target_1_phone} means arbitrarily take the first one.
* {activity.target_COMBINE_phone} means combine all with commas.
* {activity.target_ALL_phone} means give an array and I'll do some smarty-based looping in my template to get what I want.
Further, there are some situations where you might not want some recipients to see some of the other values. An example might be activity.case_id as in [16208](https://github.com/civicrm/civicrm-core/pull/16208). There maybe it makes sense to have a token that means "concatenate all the cases where the recipient has a role, and leave out the others". A similar one might apply for activity.target_display_name, where it might mean "concatenate all the with contact display names for activities where the recipient is one of the activity_contacts.
A related note that's just food for thought: In my other life, I always try to set up 2 fields when somebody asks for something multivalued. For example "services performed". Somebody might come to you asking for a couple of the different types of service you offer. You can designate one as the primary service, which is a single-valued field, which then makes reporting work because you don't run into double-counting, and then have a secondary services field that is multi-valued, which you don't use for reporting, and in emails/pdfs you can concatenate it without it being arbitrary because you would also output the primary which is single-valued. That may not be useful right now, but this is a discussion ticket.https://lab.civicrm.org/dev/core/-/issues/1486Deadlocks on acl_cache2023-09-12T18:50:06ZeileenDeadlocks on acl_cacheOne of the prevalent deadlocks we see is on the acl_contact_cache. I've found that I can replicate this locally fairly consistently just by running 2 sets of tests at once.
The thing that is somewhat odd about this is that we don't use ...One of the prevalent deadlocks we see is on the acl_contact_cache. I've found that I can replicate this locally fairly consistently just by running 2 sets of tests at once.
The thing that is somewhat odd about this is that we don't use ACLs at all so in fact the table is always empty. Notably the table is also extremely slow to truncate - despite being empty.
**What happens**
The flow we have is
1) api call to create a contact
2) when the contact is created CRM_Contact_BAO_Contact_Utils::clearContactCaches() is called. It hits a deadlock because
a) it has an FK to contact.id (& in our case it has an FK and an index seemingly)
b) it is called a lot
3) it retries and succeeds
4) we then try to add an email - the add function fails on a foreign key constraint because the earlier deadlock rolled the contact create back
**Proposals**
I think there are a number of things we can do & we should do some combo:
1) Make the delete function less expensive & less locky
a) Add an index on modified date - we are constantly deleting where modified_date < x so let's index it
b) remove the index on acl_id - we ALSO have an FK so having both is probably harmless-but-confusing
c) alter the FK on contact ID to be an index not a foreign key. This is perhaps a confusing suggestion but it is also the one that will make a real dent in these deadlocks. Remember this is a cache table not a 'persistent' table. The impact of the rows outliving the existence of the contact for a little bit is basically none because we don't do ACL lookups on non-existent contacts (this is also potentially helpful for better handling 0 or anonymous contact_id where we've had to work around this before
2) Protect against conflict through mysql locks. I think we can use a mysql lock to avoid 2 processes attempting this flush at once. I'll do a PR for this
3) How often do we REALLY need to flush those caches. Maybe we could cache a 'last flushed' key & flush no more than every 30 seconds.
4) Consider checking if the table is empty before attempting to empty it.
5) Review the places where acl cache is called from & decide how necessary there are - here is the list:
**CRM_ACL_BAO_Cache::resetCache**
CRM/Contact/BAO/Contact/Utils.php: CRM_ACL_BAO_Cache::resetCache();
CRM/Core/BAO/Cache.php: CRM_ACL_BAO_Cache::resetCache();
CRM/Utils/System.php: CRM_ACL_BAO_Cache::resetCache();
CRM/ACL/Page/EntityRole.php: CRM_ACL_BAO_Cache::resetCache();
CRM/ACL/Form/ACLBasic.php: CRM_ACL_BAO_Cache::resetCache();
CRM/ACL/Form/EntityRole.php: CRM_ACL_BAO_Cache::resetCache();
CRM/ACL/BAO/ACL.php: CRM_ACL_BAO_Cache::resetCache();
**CRM_Contact_BAO_Contact_Utils::clearContactCaches()**
CRM/Contact/BAO/Contact/Utils.php: public static function clearContactCaches($isEmptyPrevNextTable = FALSE) {
CRM/Contact/BAO/GroupContact.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches();
CRM/Contact/BAO/GroupContact.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches();
CRM/Contact/BAO/Contact.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches();
CRM/Contact/Import/Form/Preview.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches(TRUE);
bin/cli.class.php: CRM_Contact_BAO_Contact_Utils::clearContactCaches();
6) A bigger task perhaps - but permit opportunistic flushing to be turned off in favour of a cron (per group contact cache)
**Deadlock output**
LATEST DETECTED DEADLOCK
------------------------
2019-12-19 11:29:45 0x700010cd2000
*** (1) TRANSACTION:
TRANSACTION 34518715, ACTIVE 1 sec starting index read
mysql tables in use 1, locked 1
LOCK WAIT 11 lock struct(s), heap size 1136, 4 row lock(s), undo log entries 7
MySQL thread id 571, OS thread handle 123145583353856, query id 709677 localhost 127.0.0.1 drupalcivi_lkznr updating
DELETE
FROM civicrm_acl_cache
WHERE modified_date IS NULL
OR (modified_date <= '20191218222444')
*** (1) WAITING FOR THIS LOCK TO BE GRANTED:
RECORD LOCKS space id 143182 page no 3 n bits 80 index PRIMARY of table `drupalcivi_lkznr`.`civicrm_acl_cache` trx id 34518715 lock_mode X waiting
Record lock, heap no 2 PHYSICAL RECORD: n_fields 6; compact format; info bits 32
0: len 4; hex 00003c14; asc < ;;
1: len 6; hex 0000020eb6ba; asc ;;
2: len 7; hex 3b000001442ac5; asc ; D* ;;
3: SQL NULL;
4: len 4; hex 00000002; asc ;;
5: SQL NULL;
*** (2) TRANSACTION:
TRANSACTION 34518714, ACTIVE 1 sec starting index read
mysql tables in use 2, locked 2
66 lock struct(s), heap size 8400, 71 row lock(s), undo log entries 74
MySQL thread id 573, OS thread handle 123145584189440, query id 710022 localhost 127.0.0.1 drupalcivi_lkznr updating
UPDATE civicrm_contact SET contact_type = 'Organization' , contact_sub_type = NULL , email_greeting_id = NULL , postal_greeting_id = NULL , addressee_id = 3 , employer_id = 15055 WHERE ( civicrm_contact.id = 15055 )
*** (2) HOLDS THE LOCK(S):
RECORD LOCKS space id 143182 page no 3 n bits 80 index PRIMARY of table `drupalcivi_lkznr`.`civicrm_acl_cache` trx id 34518714 lock_mode X
Record lock, heap no 1 PHYSICAL RECORD: n_fields 1; compact format; info bits 0
0: len 8; hex 73757072656d756d; asc supremum;;seamusleeseamusleehttps://lab.civicrm.org/dev/core/-/issues/1481Revisiting deadlocks2022-09-01T10:27:33ZeileenRevisiting deadlocksSome time ago we added handling to the DataObject class to catch and retry deadlocks. This seems to work and be helpful in some cases but in others it doesn't work and is possibly harmful. I want to re-open the discussion on how & where ...Some time ago we added handling to the DataObject class to catch and retry deadlocks. This seems to work and be helpful in some cases but in others it doesn't work and is possibly harmful. I want to re-open the discussion on how & where we catch them & roll them back.
**Why do we have deadlocks**
Deadlocks are a 'natural' part of mysql scalability. Mysql tries to manage contention under load to the extent that can be done at the DB layer, allowing tables & rows to be locked & queuing transactions that need to use those resources to complete after that. However, in some cases mysql cannot resolve it and it returns a deadlock error. The expectation is that the application layer will handle & retry.
For example the query in this PR was causing deadlocks - https://github.com/civicrm/civicrm-core/pull/16080 - here is how I think the flow worked - note the queries are not slow but this is under high volume.
1) contact create is called by 3 different processes in pretty quick succession
2) The query to update the employer name field is called by all 3
3) the first query 'gets the lock' - the other 2 get shared locks while they wait
4) the first query finishes. Neither of the other 2 can get the exclusive lock as they both have shared locks
5) one is reverted & a deadlock is thrown
6) the deadlock is caught in packages/DB/Dataobject
7) the Dataobject code retries the query and succeeds. The transaction succeeds.
**When don't we do a good job with deadlocks**
The above scenario is good because once the deadlock was resolved we were able to retry and it worked. However, it turns out that mysql often rolls back more than just the query it was doing when it decided there was a deadlock. Per https://dev.mysql.com/doc/refman/8.0/en/innodb-deadlock-detection.html " InnoDB automatically detects transaction deadlocks and rolls back a transaction or transactions to break the deadlock."
We are seeing a consistent pattern where under high volume we see deadlocks on ```INSERT INTO civicrm_email``` - via contact.create api (called in turn from a job api by drush). The INSERT email fails but on retry it hits a constraint error - because unknown to the php layer the ```INSERT INTO civicrm_contact ``` statement was also rolled back.
In this case the DB state is hopefully still consistent as the failure should have triggered more rollbacks but it's at least theoretically possible to have lost data in the rollback & then have the query succeed when retried - so the database is in an inconsistent state.
My high level conclusion is that we should be catching deadlocks & retrying higher up the stack - but I'm still trying to figure out where.https://lab.civicrm.org/dev/core/-/issues/1437Creating relationship with start date in future incorrectly shows relationshi...2023-01-17T01:36:45Zellen_compucorpCreating relationship with start date in future incorrectly shows relationship as activeOverview
----------------------------------------
Creating relationship with start date in future incorrectly shows relationship as active
Reproduction steps
----------------------------------------
1. Go to a contact record, then go to...Overview
----------------------------------------
Creating relationship with start date in future incorrectly shows relationship as active
Reproduction steps
----------------------------------------
1. Go to a contact record, then go to the relationships tab
1. Create a new relationship of any type
1. Give the relationship a start date that is in the future
1. Save the new relationship
Current behaviour
----------------------------------------
The relationship is saved correctly, but displays within 'current relationships' in the top half of the relationships tab.
![6AD41E8B-5DD7-4497-84A5-96600B621E9A](/uploads/a43484418ac5b913033190c9e3a52f73/6AD41E8B-5DD7-4497-84A5-96600B621E9A.jpg)
Expected behaviour
----------------------------------------
As the relationship has not yet started, the relationship should show as inactive up until the start date; at which point it should show as current.
Environment information
----------------------------------------
* __Browser:__ Chrome
* __CiviCRM:__ 5.19 (also tested on 5.15 on this test site and contact record: https://civicrm.demo.civihosting.com/civicrm/contact/view?reset=1&cid=98)
* __PHP:__ _7.0
* __CMS:__ Drupal
Comments
----------------------------------------
Contact record from my testing on a demo site: https://civicrm.demo.civihosting.com/civicrm/contact/view?reset=1&cid=98
cc @jamienovick1 @shitijghttps://lab.civicrm.org/dev/core/-/issues/1408The DAO "FreshnessTest" in the jenkins test suite does nothing because setup....2023-01-08T05:19:21ZDaveDThe DAO "FreshnessTest" in the jenkins test suite does nothing because setup.sh runs before itThe test itself works (CRM_Core_CodeGen_FreshnessTest::testDAOs()), but because in jenkins runs setup.sh gets run near the start of the build, a DAO will appear to be updated even if the git tree doesn't have the updated DAO.
The purpos...The test itself works (CRM_Core_CodeGen_FreshnessTest::testDAOs()), but because in jenkins runs setup.sh gets run near the start of the build, a DAO will appear to be updated even if the git tree doesn't have the updated DAO.
The purpose of the freshness test seems to be to make sure you don't forget to include the DAO in PRs when you make changes to an xml schema to add/change/remove a field in core tables. But it's not doing that currently in jenkins runs because it's always comparing against DAOs after setup.sh/GenCode.php has run.
I'm not sure if there's an easy fix, or maybe the test should be removed and replaced with some developer documentation? It wouldn't even work in a local test suite run if you've run GenCode.php, which you would have had to do to physically try out your schema changes.https://lab.civicrm.org/dev/core/-/issues/1403Change "Contribution Received Date" to be "Contribution date" where is exists...2024-01-28T01:17:26ZJamie Novick - CompucoChange "Contribution Received Date" to be "Contribution date" where is exists and basically sort this Contribution dates mess out once and for allOk... sorry if this comes across as a bit of a rant but as a qualified accountant this has been driving me crazy for years and I think we've been skirting around this issue for sometime. I wanted to put in a gitlab to start a discussion ...Ok... sorry if this comes across as a bit of a rant but as a qualified accountant this has been driving me crazy for years and I think we've been skirting around this issue for sometime. I wanted to put in a gitlab to start a discussion to allow us to sort this out once and for all.
**Overview:**
----------------------------------------
CiviCRM has a core field on the contribution called "receive_date". This shows up on various screens and has led to significant debate about whether it should be a required field or not, and what it's real use case is. For example discussions below where debate has raged on how it should work:
https://lab.civicrm.org/dev/core/issues/1057
https://lab.civicrm.org/dev/core/issues/680
https://lab.civicrm.org/dev/core/issues/1140 - slightly different but related re: accruals accounting
https://lab.civicrm.org/dev/core/issues/1402
**The issue:**
----------------------------------------
So... there are a few problems with this field:
a) The idea of a "received date" is a bit meaningless (and confusing to users) when you have a pending contribution. You haven't quite received anything yet.
b) The idea of a "received date" conflicts with the payments dates if you have multiple payments (or could even be made to be different from the payment date) again which is nonsensical.
I assume contribution received date was a field that pre-dated the CiviCRM accounting implementation and hence perhaps hasn't kept up with the reality of the situation. In any case I believe this is causing some real issues now and should be sorted out.
Note the field is used:
1) As the date when initial accounting transactions are made. i.e. create a new pending contribution it will be the date of the Debit and Credits / transaction recorded in CiviCRM and hence by default it is the "revenue recognition date" for account purposes for the income.
This then leads to a multitude of other issues:
a) If I "complete" a pending membership contribution for a new membership by recording a payment, what should the start date of the membership be? (see: https://lab.civicrm.org/dev/core/issues/1402)
b) We've then introduced another field called "revenue_recognition_date", which should actually be field 1 above - as that is when we are recognising the transactions. We shouldn't have another field for this.
c) For some reason we now have a field on the invoice called "invoice date" which prints todays date which again is incorrect as it should be the same as item 1 above. The invoice should reflect the revenue recognition date. https://civicrm.stackexchange.com/questions/21128/how-do-i-print-the-received-date-on-an-invoice
**Proposed solution:**
----------------------------------------
The solution is relatively simple. We should change the "received_date" field to simply be the "Contribution date".
| Invoicing? | Contribution status | What the field means | Changes required to CiviCRM |
| ------ | ------ | ------ | ------ |
| Not using invoices | Pending | Represents the date the contribution was recorded on the system. | When later marking the contribution as completed give users the option to update the date or better still don't offer the option to complete a contribution and simply only allow users to enter the payment separately.
| Not using invoices | Completed | Represents the date the contribution was recorded on the system but also record a payment on the same date.
| Using invoices | Pending | Represents the date the invoice was recorded on the system and hence the invoice date and the revenue recognition date. | We should update the invoices to use this date as the invoice date. This field should not be updated when marking the invoice as complete (or better still don't offer the option to complete a contribution and simply only allow users to enter the payment separately.)
| Using invoices | Completed | Represents the date the invoice was recorded on the system but also the date the payment for that invoice was received. Ideally we would split the invoice and payment sections from each other on the "create new" form.
**Future:**
----------------------------------------
At some date in the future it would then be good to properly decouple payments from contributions and get rid of the terrible contribution status field once and for all.
Basically the contribution would reflect an order or invoice object, i.e. the obligation to pay for something. The payment transaction then would be separate and would represent the actual payment of that obligation. Accounting software has done this correctly for years so it's about time CiviCRM did too!
@mattwire @eileen @KarinG @dylan-circle @jaapjansma @JoeMurray @ErikHommel @fabian_SYSTOPIA and a few Compucorp names also - @omar_compucorp @Miya27
I'm sure you will all have opinions on this...https://lab.civicrm.org/dev/core/-/issues/1380No easy way to import signatures to petition2022-11-30T19:32:57ZCoreyBurgerNo easy way to import signatures to petitionSo there is no easy way to mass import signatures to a petition.
When looking at Petitions on Campaign > Dashboard > Petitions on the right side under the more option, there should be an option to "import signatures", which should be a...So there is no easy way to mass import signatures to a petition.
When looking at Petitions on Campaign > Dashboard > Petitions on the right side under the more option, there should be an option to "import signatures", which should be a custom version of the Import Activities.
As a workaround, I had to manually lookup how to import it, include the required numeric fields for source_record_id and activity_id.
CiviCRM 5.15 on Wordpresshttps://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/1321group.get API (v3- but lets fix for v4) fails to find groups of one group_typ...2023-08-04T20:49:54ZRichgroup.get API (v3- but lets fix for v4) fails to find groups of one group_type if group has multiple typesWe used (5.15) to be able to filter for mailing groups with the API and now we can't. I'm not sure when this came in.
e.g. on a buildkit at 58e7f004e2
```
cv api Group.get return='id,title,group_type'
```
Returns
```
{
"is_error":...We used (5.15) to be able to filter for mailing groups with the API and now we can't. I'm not sure when this came in.
e.g. on a buildkit at 58e7f004e2
```
cv api Group.get return='id,title,group_type'
```
Returns
```
{
"is_error": 0,
"version": 3,
"count": 4,
"values": {
"1": {
"id": "1",
"title": "Administrators",
"group_type": [
"1"
]
},
"2": {
"id": "2",
"title": "Newsletter Subscribers",
"group_type": [
"1",
"2"
]
},
"3": {
"id": "3",
"title": "Summer Program Volunteers",
"group_type": [
"1",
"2"
]
},
"4": {
"id": "4",
"title": "Advisory Board",
"group_type": [
"1",
"2"
]
}
}
}
```
But
```
cv api Group.get return='id,title,group_type' group_type='Mailing List'
cv api Group.get return='id,title,group_type' group_type=2
```
returns none.
<del>On 5.15 Mailing groups are returned properly.</del>https://lab.civicrm.org/dev/core/-/issues/1275Batch update for cases gives a warning and then fatal error if only one case ...2022-09-20T11:04:16ZDaveDBatch update for cases gives a warning and then fatal error if only one case is in the listI also see this on dmaster.demo, and I'm still trying to see the exact triggers to reproduce and when it started, but what seems to be a consistent one is:
1. First set up a profile that has a case field in it, e.g. a profile that just ...I also see this on dmaster.demo, and I'm still trying to see the exact triggers to reproduce and when it started, but what seems to be a consistent one is:
1. First set up a profile that has a case field in it, e.g. a profile that just has case status.
2. Do a find cases that results in only one search result.
3. Select it and choose update multiple cases from the actions dropdown.
4. You'll get a warning like `Notice: Undefined property: CRM_Core_DAO::$case_id in CRM_Core_Form_Task::preProcessCommon() (line 139 of /srv/buildkit/build/dmaster/web/sites/all/modules/civicrm/CRM/Core/Form/Task.php)`.
5. If you continue and select your profile, you then get a fatal error where it's obvious the problem is that it wasn't able to obtain the case id (the `IN ()` part):
```
[message] => DB Error: syntax error
[mode] => 16
[debug_info] =>
SELECT contact.id as contactId, civicrm_case.id as componentId, contact.sort_name as sort_name, email as email
FROM civicrm_case as civicrm_case
INNER JOIN civicrm_case_contact ccs ON (ccs.case_id = civicrm_case.id)
INNER JOIN civicrm_contact contact ON ( contact.id = ccs.contact_id ) LEFT JOIN civicrm_email email ON ( contact.id = email.contact_id AND email.is_primary = 1 )
WHERE civicrm_case.id IN ()
GROUP BY civicrm_case.id, contact.id
```https://lab.civicrm.org/dev/core/-/issues/1197Participants counted even when marked as "0" on price set line item2023-06-18T00:47:30ZphilmorbruParticipants counted even when marked as "0" on price set line item[This issue was originally reported in Jira as [CRM-20784](https://issues.civicrm.org/jira/browse/CRM-20784). Bringing it here to make sure it doesn't get lost as it continues to be a bug in 5.13.5]
When creating a price set, you can se...[This issue was originally reported in Jira as [CRM-20784](https://issues.civicrm.org/jira/browse/CRM-20784). Bringing it here to make sure it doesn't get lost as it continues to be a bug in 5.13.5]
When creating a price set, you can set the number of participants who should be counted for that price option. In this use case, I was setting up a price option for a table (with a participant count of 8) and for a table guest (with a participant count of 0). This way, the person purchasing the table could choose whether or not to register the actual names and emails of the additional table guests without paying more or having them count toward the total number. When testing, the table price option worked as expected, counting 8 participants, but the table guest still counted as 1. If there's an option to set the participant count to 0, then it should not calculate accordingly in the Actual Participant Count.https://lab.civicrm.org/dev/core/-/issues/1195Links in PayPal Standard recurring contribution confirmation email don’t work2023-11-23T07:47:01ZBobSLinks in PayPal Standard recurring contribution confirmation email don’t workRef: https://lab.civicrm.org/dev/core/issues/760
Contribution confirmation emails for recurring PayPal Standard contributions contain three links that are non-functional.
**Link 1: “This is a recurring contribution. You can cancel futu...Ref: https://lab.civicrm.org/dev/core/issues/760
Contribution confirmation emails for recurring PayPal Standard contributions contain three links that are non-functional.
**Link 1: “This is a recurring contribution. You can cancel future contributions by visiting this web page.”**
* Two questions are shown which seem inappropriate for a front-end page:
1. Send cancellation request to PayPal Standard ?
2. Notify Contributor?
* On submit:
* If Send Cancellation = Yes:
* Green-background status message: "The recurring contribution could not be cancelled."
* No change to recurring status in Civi or PayPal.
* If Send Cancellation = No:
* Green-background status message: “The recurring contribution of 5.20, every 1 month has been cancelled.”
* Contact record shows recurring contribution is cancelled
* Merchant and donor PayPal accounts continue to show active recurring transaction.
**Link 2: “You can update billing details for this recurring contribution by visiting this web page.”**
* There are only Save and Cancel buttons, but no form fields.
* Submit: "There was some problem updating the billing details"
**Link 3: “You can update recurring contribution amount or change the number of installments for this recurring contribution by visiting this web page.”**
* Recurrence Period is indicated as "(every )" rather than ‘(every month)”.
* If not logged in:
* Popup: "Access is denied. Ensure that you are still logged in and have permission to access this feature."
* On submit: Blank screen.
* If logged in:
* On submit:
* Displays Find Contributions page with no error indicated.
* No change observed in either merchant or donor PayPal accounts. Both show an active recurring payment.
**Analysis**
(I’ll focus on link 2 as an example, and assume that none of these 3 links should appear as their functions are unsupported by PayPal (?) for PayPal Standard payments)
1. Sending of confirmation email is initiated by Paypal performing an IPN POST.
2. CRM_Contribute_BAO_Contribution->composeMessageArray calls CRM_Core_Payment_PayPalImpl->subscriptionURL to form the problematic URLs.
3. CRM_Core_Payment_PayPalImpl->subscriptionURL attempts to determine if the payment processor supports each operation by doing, for example “if (!$this->supports('updateSubscriptionBillingInfo'))” which in turn returns method_exists($this, supportsUpdateSubscriptionBillingInfo).
4. CRM_Core_Payment (parent class of CRM_Core_Payment_PayPalImpl) does indeed contain function supportsUpdateSubscriptionBillingInfo which returns “method_exists(CRM_Utils_System::getClassName($this), 'updateSubscriptionBillingInfo')”.
5. CRM_Core_Payment_PayPalImpl does include function updateSubscriptionBillingInfo, although it begins with “if ($this->isPayPalType($this::PAYPAL_PRO)) “ and therefore simply returns false for PayPal Standard or Express
6. Template "Contributions - Receipt (on-line)" includes the problematic URL section based on the existence of $updateSubscriptionBillingUrl (for example).
**Conclusion**
* Step 3 seems to be a problem as it tests only for the existence of supportsUpdateSubscriptionBillingInfo (found in the parent class) rather than calling that function to get a meaningful result.
* Step 4 seems to be a problem because while the function 'updateSubscriptionBillingInfo' does indeed exist, it does nothing if the payment processor is PayPal Standard.
Behavior confirmed in 5.13.4 and in master (5.18.alpha1).https://lab.civicrm.org/dev/core/-/issues/1191CiviCRM reaching MySQL join limit2023-11-26T16:41:52ZJGauntCiviCRM reaching MySQL join limitI've seen a similar error on a few websites where there is a lot of custom data which can throw up a few issues and it was mentioned I should write up an issue here to see whether it is a common issue in the community.
All current versi...I've seen a similar error on a few websites where there is a lot of custom data which can throw up a few issues and it was mentioned I should write up an issue here to see whether it is a common issue in the community.
All current versions of MySQL and MariaDB have a join limit of 61 and Civi needs to join a lot of tables together (dependant on how much custom data you have). This can cause problems when you have a site with a lot of custom data sets as this join limit is reached which prevents Civi from working as expected.
An example of this would be yesterday where I tried to load up a contact's activities but, because of the amount of custom data sets on the site, I was given an AJAX error and could not view the contact's activities.
After doing a bit of debugging, I found out the reason was because the join limit of 61 had been reached.
I managed to resolve the issue by disabling some unused custom data sets but I am wondering if this is quite a common issue with some bigger sites.
My thinking is that as time goes on, the database will naturally get bigger as more is added to it and the issue will come up more and more.
Looking forward to hearing some thoughts and suggestions on this!https://lab.civicrm.org/dev/core/-/issues/1129Duplicate records on CRM_Report_Form_Member_Detail2022-09-26T00:11:17ZandyburnsDuplicate records on CRM_Report_Form_Member_DetailOn CRM_Report_Form_Member_Detail, the rows get duplicated. They are not actual duplicates, it is the same contact ID and they do not have duplicate memberships.
![5b7f716e](/uploads/6ae639a21fd22a9b5fc47c1d1edf66be/5b7f716e.png)
On 5.13.4On CRM_Report_Form_Member_Detail, the rows get duplicated. They are not actual duplicates, it is the same contact ID and they do not have duplicate memberships.
![5b7f716e](/uploads/6ae639a21fd22a9b5fc47c1d1edf66be/5b7f716e.png)
On 5.13.4https://lab.civicrm.org/dev/core/-/issues/1125Custom fieldsets participants added twice2023-02-15T15:13:29ZwdecraeneCustom fieldsets participants added twiceWhen editing a participant, some custom fieldsets are added twice to the participant edit form. When saving, only the data in the second fieldset is saved (so users think nothing happened when they added the data in the first instance of...When editing a participant, some custom fieldsets are added twice to the participant edit form. When saving, only the data in the second fieldset is saved (so users think nothing happened when they added the data in the first instance of the fieldset).
This only happens with fieldsets linked to a specific event type.
In templates/CRM/Event/Form/Participant.tpl:
```smarty
CRM.buildCustomData( '{$customDataType}', null, null );
{if $eventID}
CRM.buildCustomData( '{$customDataType}', {$eventID}, {$eventNameCustomDataTypeID} );
{/if}
{if $eventTypeID}
CRM.buildCustomData( '{$customDataType}', {$eventTypeID}, {$eventTypeCustomDataTypeID} );
{/if}
```
* First buildCustomData : adds all custom field sets, except those linked to a specific event id
* Second buildCustomData : adds all custom field sets linked to a event id
* Third buildCustomData : adds all custom field sets linked to a specific event type id
So the combination of the first and third one is the cause of fieldsets added twice.
Solution? Altering template, changing arguments, fixing getTree() in CRM/Core/BAO/CustomGroup.php, ... ?
See also
* https://issues.civicrm.org/jira/browse/CRM-10983
* https://issues.civicrm.org/jira/browse/CRM-20633https://lab.civicrm.org/dev/core/-/issues/1116Changing a civicase activity's label breaks the max_instances check2023-02-23T14:16:28ZDaveDChanging a civicase activity's label breaks the max_instances check1. Configure a case type so that some activity type (other than open case) has a max_instances set.
2. Create a case.
3. Change the activity type's label.
4. On manage case, create a second activity of that type.
5. You should get the ma...1. Configure a case type so that some activity type (other than open case) has a max_instances set.
2. Create a case.
3. Change the activity type's label.
4. On manage case, create a second activity of that type.
5. You should get the max_instances warning but it doesn't.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/1036Deprecate Tell a Friend2022-12-06T23:14:44ZAndie HuntDeprecate Tell a FriendWho tells their friends anymore? This really should be removed or deprecated.
@JonGold came up with this idea and @bgm thought it would be worth opening an issue for it.Who tells their friends anymore? This really should be removed or deprecated.
@JonGold came up with this idea and @bgm thought it would be worth opening an issue for it.https://lab.civicrm.org/dev/core/-/issues/960Proposal: Change supervised dedupe rules to name(s) OR email2023-03-18T04:52:01ZJonGoldProposal: Change supervised dedupe rules to name(s) OR emailI've done countless Civi trainings on deduping, many at CiviCamps and other "official" places. And each time, I need to explain that "yes, unsupervised rules should be more strict than supervised rules. Also yes, the default individual...I've done countless Civi trainings on deduping, many at CiviCamps and other "official" places. And each time, I need to explain that "yes, unsupervised rules should be more strict than supervised rules. Also yes, the default individual supervised rule is far more strict than the default unsupervised rule."
Surely I'm not the only one who feels the same - and yet none of us noticed, because before Civi 5.3/CRM-20565, the supervised rule was basically unused.
Now, there's a code comment that says, [CRM-20565 - Need a good default matching rule before using the dedupe engine for checking on-the-fly.](https://github.com/civicrm/civicrm-core/blob/1eca040f3f6efa0a8a43575cd0a08bae242a95f3/templates/CRM/Contact/Form/Contact.tpl#L334).
I'm proposing we fix this by changing the default supervised rule. The current on-the-fly checking is both a) a default, and b) very poor UX.
I'm also proposing that we do this as the new default for everyone. I'm mindful of the changes to existing systems, and we should probably put in an upgrade note or something - but honestly anyone who's never changed the default supervised rules isn't very likely to be using them.JonGoldJonGold