Add createdon/editedon timestamps to element and category tables

[pbowyer]

What does it do?

MODX tracks when resources, users, and settings were created and last modified, but not elements. There's no way to tell when a snippet was last changed or which templates haven't been touched in years.

As a developer, this information is useful. This PR adds createdon and editedon columns to chunks, snippets, plugins, templates, TVs, and categories.

How does it work?

Both columns are int Unix timestamps with a default of 0, matching modResource. They're set in PHP rather than by MySQL:

• createdon is set in modElement::save() and modCategory::save() when the row is new, unless a value was already supplied.
• editedon is set on every save of an existing row.
• Duplicating an element gives the copy a fresh createdon and resets editedon to 0.

Existing rows get 0 for both after the upgrade.

editedon means "the last time this row was saved", which differs from modResource, where processors set it. I chose this deliberately, so it also changes when MODX saves an element or category outside the update processors: moving a category with the Sort processor, orphaning elements when their category is removed, resyncing a static element's file content, and package installs. For static elements, it reflects when MODX last synced the file to the database, which may differ from when the file itself changed.

To stop static scripts being re-saved on every load, modScript::getFileContent() now trims newlines as well as spaces. Before, the newline after <?php in the file survived the trim, so the file content didn't match the stored content and getContent() saved the element on every load. After upgrading, static snippets and plugins whose stored content picked up that newline will be re-saved once on first load and get an editedon stamp.

The upgrade script is setup/includes/upgrades/common/3.3.0-element-timestamps.php, included from setup/includes/upgrades/mysql/3.3.0-pl.php.

How to test

1. Run MODX setup upgrade on an existing install and check the six tables have createdon and editedon columns
2. Create a new element in the manager and check createdon is set
3. Edit it and check editedon is set and createdon is unchanged
4. Duplicate it and check the copy has a new createdon and editedon of 0

The test suite covers createdon being set on create for chunks and categories, createdon being preserved on update, duplicates resetting both timestamps, and unchanged static snippets and plugins not being re-saved on load.

Related issue(s)/PR(s)

#13857 was a previous PR to do this. It added createdby and editedby as well, and as it went the scope expanded. I have kept my PR focused because I am not using the other features. It ran into problems with static elements, and I think I have taken a pragmatic approach to solving that.

#13770 #13626 #13641 #2027

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 22.20%. Comparing base (5a484de) to head (e599db2).
⚠️ Report is 33 commits behind head on 3.x.

Files with missing lines	Patch %	Lines
core/src/Revolution/modCategory.php	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                3.x   #16930      +/-   ##
============================================
+ Coverage     21.67%   22.20%   +0.52%     
- Complexity    10786    10827      +41     
============================================
  Files           566      567       +1     
  Lines         33149    33300     +151     
============================================
+ Hits           7186     7394     +208     
+ Misses        25963    25906      -57     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[smg6511]

@pbowyer Hey Peter - Absolutely agree the timestamp additions should be done. However, for consistency with the current data structure, I'd much rather see them implemented as unix timestamps.

For next-gen MODX (presumable 4.0) the optimal date/time storage type can be debated and, if changed, implemented across the whole codebase. Note that in the context of web apps, the 32-bit limitation is really a non-issue as, from my research, you're pretty much only going to find legacy non 64-bit hardware in industrial settings, especially 12 years from now.

[pbowyer]

Thanks for your review @smg6511. I disagree with you (I would improve the column type now for new additions, so there's less to migrate in MODX v4) but I've changed them to be Unix timestamps as requested.

[smg6511]

@pbowyer - Hey Peter, thanks for being flexible and adjusting to UTs for this PR. I've confirmed that, for the most part, timestamps are recorded as expected. I have a handful of initial review questions and requests to share ahead of a more final review:

1. I'm unsure about the value of adding these cols to the association tables, especially the TVRs one. I'd like to get your take on how it'd be useful. @opengeek @Mark-H and others: It'd be great to get your opinions on this as well.
2. Same as above goes for the Plugin Event table.
3. With Snippets and Plugins, after the first save something seems to trigger the save again (or maybe it's just the reloading of the page) — resulting in editedon being recorded as well (a second or two later than createdon). It'd be ideal to find a way for this to not happen.
4. Nuance 1: I think logic should also be added for the Duplicate processors, adding a beforeSave method to reset the duplicated element's createdon to the current time() and editedon to empty.
5. Nuance 2: This may or may not be a job for another PR but I noted that when a static element's content is externally edited you have to at least revisit/reload the element's editing page for editedon to be updated. I was wondering if clearing cache could also trigger this update. This may be too fine a point, but just wanted to at least bring it up.

[pbowyer]

• I'm unsure about the value of adding these cols to the association tables, especially the TVRs one. I'd like to get your take on how it'd be useful. @opengeek @Mark-H and others: It'd be great to get your opinions on this as well.

• Same as above goes for the Plugin Event table.

Two I did because I have a use for them: for plugin events it's helpful to see when new events were added to a plugin. The same to a lesser extent for site_tmplvar_access because it helps with auditing direct from the database.

The other two (TVRs and TVTpls) I included for consistency, not because I currently would use that field.

• With Snippets and Plugins, after the first save something seems to trigger the save again (or maybe it's just the reloading of the page) — resulting in editedon being recorded as well (a second or two later than createdon). It'd be ideal to find a way for this to not happen.

I cannot reproduce this. Do you perhaps still have the ON UPDATE CURRENT_TIMESTAMP present after changing the datetime column type to int?

• Nuance 1: I think logic should also be added for the Duplicate processors, adding a beforeSave method to reset the duplicated element's createdon to the current time() and editedon to empty.

Good catch, added in 97b73d4.

• Nuance 2: This may or may not be a job for another PR but I noted that when a static element's content is externally edited you have to at least revisit/reload the element's editing page for editedon to be updated. I was wondering if clearing cache could also trigger this update. This may be too fine a point, but just wanted to at least bring it up.

I purposefully kept this out of scope of this PR because it's what stopped the former PR (#13857 (comment) onwards). Addressing it in a follow-on PR will keep the discussion focused and allow the side-discussions static elements seem to generate(!) to happen without affecting this change.

[smg6511]

@pbowyer - Hello again! Ok, as I continue to look this over, I realize I failed to specify that the effect I was seeing with Snippets and Plugins seemingly double-saving had to do with static ones. That's probably why you couldn't replicate what I was describing (?).

Thanks for making the changes for the Duplicate processor, that's working as expected.

On the association tables, I'm not saying “no” but just want you to consider whether these changes have broader appeal. If it's pretty niche or for mostly a personal need, then it might be the job of a custom component/plugin or and Extra (thinking of auditing).

[pbowyer]

Hi @smg6511 and thanks for the bug report. I was able to reproduce and fix the problem, it was happening because static Snippet/Plugin files retained a leading newline after stripping <?php. MODX interpreted the content as changed, saved the element during reload, and populated editedon.

Tangentially the same extra newline has caused us problems when using Gitify, as it leads to unexpected changes after exporting an imported snippet saved via the Manager. I'd never investigated, but this explains why it happens and the fix will sort that too.

On the association tables, I'm not saying “no” but just want you to consider whether these changes have broader appeal. If it's pretty niche or for mostly a personal need, then it might be the job of a custom component/plugin or and Extra (thinking of auditing).

I don't think that's for me to consider. I've presented a PR and @opengeek needs to make a call on their inclusion or not as he's steering the direction of the project.

[smg6511]

@pbowyer - Ok, so let's work on moving this forward without the addition to the association tables. I talked with Jason a while back and, while he didn't have a strong opinion on this, he said that it was a bit much adding them there. I personally think that dates should only be persisted in the db for the objects we create and tracking activity re peripheral tables' data might better be left to an auditing Extra/CMP.

[pbowyer]

Hi @smg6511 I've removed the association timestamps and rebased the branch too.

[smg6511]

@pbowyer - Hey Peter, thanks for the update.

@opengeek @JoshuaLuckers @Mark-H - and others who were/are involved in this and the original solution: The small issue of editedon not being tracked when a static file is updated outside of MODX still stands, but I think that's a separate issue to tackle in a follow-up PR. Everything else is in order here, so I say let's move forward.

[smg6511]

Re-tested and works.

[opengeek]

Thank you for your work on this, Peter. The model and processor changes look good and the maps match a regeneration from the schema. One significant issue with the setup side, though, was found while running a build + upgrade against a snapshot.

Blocking: the upgrade script never runs

setup/includes/upgrades/common/3.2.1-element-timestamps.php is not included by anything. modInstallVersion::_getUpgradeScripts() scans upgrades/mysql/, and each mysql/X.Y.Z-pl.php there includes the relevant common/ scripts. Without a wrapper the columns are never added.

After running the upgrade, any element or category save fails, and since xPDO selects columns explicitly, element grids fail:

Unknown column 'modSnippet.createdon' in 'SELECT'
Unknown column 'createdon' in 'INSERT INTO'

Separately, the 3.2.1 prefix is wrong for the release this ships in. version_compare('3.2.2-pl', '3.2.1-element-timestamps', '<') is false, thus installs on 3.2.1 or 3.2.2 would skip it even if it were wired up.

Fix (verified locally: columns added to all six tables, saves work, createdon set on create, editedon set on update):

1. Rename to common/3.3.0-element-timestamps.php
2. Add setup/includes/upgrades/mysql/3.3.0-pl.php:

<?php
/**
 * Specific upgrades for Revolution 3.3.0-pl
 *
 * @var modX $modx
 * @package setup
 * @subpackage upgrades
 */

/* run upgrades common to all db platforms */
include dirname(__DIR__) . '/common/3.3.0-element-timestamps.php';

Cleanup Requests/Considerations

• PR description is out of date. It describes datetime columns with ON UPDATE CURRENT_TIMESTAMP, NULL for existing rows, and association tables. The implementation is int Unix timestamps set in PHP with default 0, and association tables were dropped. Please freshen this in order to maintain accurate merge history.
• editedon is stamped on every non-new save(), including category moves via the Sort processor, elements orphaned by a category removal, static file resyncs in getContent(), and package installs. That fits the "last time the row changed" semantic you chose, but it differs from modResource, where processors set it. I wanted to note this to ensure this behavior is a deliberate decision.
• modScript::getFileContent() trim change: Good fix for the leading newline issue. One-time side effect after upgrade: existing static scripts that stored the newline will be re-saved once on first load and get an editedon stamp. I think this is fine, but worth a line in the description.
• Whitespace-only changes remain on modPluginEvent, modTemplateVarResource*, modTemplateVarTemplate maps and a blank line in PasswordResetToken.php and its test, left over from the reverted association table commit. Harmless, but could be dropped to keep this clean.

[pbowyer]

Hi @opengeek,

Thanks for running the build and upgrade against a snapshot, and for the fix. I've renamed the script to common/3.3.0-element-timestamps.php and added mysql/3.3.0-pl.php to include it.

On editedon: yes, stamping it on every save of an existing row is deliberate. I wanted it to mean "the last time this row changed", whichever code path saved it, so category moves, orphaning, static resyncs and package installs all count. I've spelled this out in the description, along with the one-off re-save of static scripts that stored the leading newline.

I've also rewritten the description to match the implementation, and dropped the leftover whitespace changes from the maps, PasswordResetToken.php and its test.