Uploaded image for project: 'CiviCRM'
  1. CiviCRM
  2. CRM-20178

Writes to timestamp fields are silently ignored by $dao->save()

    Details

    • Type: Bug
    • Status: Done/Fixed
    • Priority: Minor
    • Resolution: Fixed/Completed
    • Affects Version/s: 4.7.16
    • Fix Version/s: 4.7.19
    • Component/s: Core CiviCRM
    • Labels:
    • Versioning Impact:
      Patch (backwards-compatible bug fixes)
    • Documentation Required?:
      None
    • Funding Source:
      Contributed Code
    • Verified?:
      No

      Description

      To reproduce:
      See CRM-20173.

      The cause:
      Timestamp fields are explicitly excluded from $dao->save() operations.

      Consequences:

      • Erosion of developer trust in fundamental core utilities.
      • Development of needless workarounds (and possibly more than one pattern of workaround), leading to code bloat, decreased maintainability, unnecessary complexity.
      • Hair ripped out, laptops thrown, etc.

      Challenges:
      The current behavior is nonsensical, unexpected, and arbitrary, but because the code is so old, poorly documented, and "deep" in the stack, changing it is scary. What other code might depend on this unusual behavior? What might we unwittingly break?

      Potential steps forward:
      Mostly gleaned from initial discussion on CRM-20173.

      • Grep the xml directory to get a sense of how many timestamps are in core, and hence which fields/entities might be affected.
      • Generate a warning any time someone attempts to write to a timestamp field. This will communicate better to the developer what's going wrong, and it provides some visibility into scenarios that would be affected by a hypothetical fix. (If the warning is really common, maybe only show it if debug mode is enabled.)
      • Write the patch to fix saving of timestamps. Get a few different orgs/consultants to run it for a month in production. (The sites using it should be a good cross-section of configurations/components.) If it works well, then merge it into mainline.
      • Grandfather in all the existing timestamp fields, and switch them over 1-by-1 (as needed). Also, generate warnings. Pseudocode: https://gist.github.com/totten/e56863c79b47367865d60143891c08ef
      • The following is based on the uninvestigated assumption that this behavior was put in place to prevent developers from writing to timestamps which are automatically set to the current timestamp on update: Add metadata to the xml schema declarations and logic to Gencode.php to set a property on fields that shouldn't be updated, and check that instead of the field type.

      Suspicious code to consider:

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                Unassigned
                Reporter:
                pittstains Frank J. Gómez
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: