[#48282] - [6.2] Fix ContentHistory::getSha1 object mutation and nested property flattening
- Pending
- Medium
- Build: 5.4-dev
- # 48282
- Diff
- bhuvan-somisetty:fix-contenthistory-getsha1-48281
User tests: Successful: Unsuccessful:
Pull Request resolves #48281.
- I read the Generative AI policy and my contribution is either not created with the help of AI or is compatible with the policy and GNU/GPL 2 or later.
Summary of Changes
This PR fixes two bugs in Joomla\CMS\Table\ContentHistory::getSha1():
- Prevent caller object in-place mutation: When
$jsonDatais passed as an object (e.g., fromVersioning::store()),$objectis now decoupled before unsetting ignored properties (modified,modified_by,checked_out,version,hits,path). This prevents stripping properties from the caller's live in-memory Table/data instance. - Correct nested sub-property normalization: Fixed the inner loop in
getSha1()to assign normalized values to$value->$subName(on the nested object) instead of$object->$subName. This prevents flattening nested JSON column properties onto the root object, avoids clobbering top-level properties (liketitleoralias) when nested keys collide, and ensures nested JSON column values are normalized. - Safety for non-object payloads: Added a guard returning
''when payload cannot be decoded into an object. - Unit Tests: Added
tests/Unit/Libraries/Cms/Table/ContentHistoryTest.phpcovering input immutability, nested property normalization, key collision safety, and deterministic hash calculation.
Testing Instructions
Run the unit tests:
libraries/vendor/bin/phpunit tests/Unit/Libraries/Cms/Table/ContentHistoryTest.phpOr test programmatically with a sample object containing nested parameters:
$data = (object) [
'title' => 'Article Title',
'modified_by' => 42,
'version' => 1,
'params' => (object) [
'title' => 'Param Header',
'show_title' => 1,
],
];
$historyTable = new \Joomla\CMS\Table\ContentHistory($db);
$contentType = new \Joomla\CMS\Table\ContentType($db);
$hash = $historyTable->getSha1($data, $contentType);Actual result BEFORE applying this Pull Request
$data->modified_byand$data->versionare removed from the caller's live object$data.$data->titleis overwritten with'Param Header'.$data->show_titleis injected into the root object$data.$data->params->show_titleremains unconverted integer1instead of normalized string'1'.
Expected result AFTER applying this Pull Request
$dataremains completely unmodified in-place ($data->modified_byand$data->versionremain intact).$data->titleis preserved as'Article Title'.- Root object is not polluted with sub-properties.
- Nested properties are normalized in the calculated hash.
- All unit tests pass.
Link to documentations
Please select:
-
Documentation link for guide.joomla.org:
-
No documentation changes for guide.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
Signed-off-by: bhuvan-somisetty somisettybhuvan5@gmail.com
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries Unit Tests |
| Labels |
Added:
Unit/System Tests
PR-5.4-dev
|
||
cc @bembelimen
@brianteeman Fair question!
In the snippet in the testing instructions, 'title' => 'Param Header' inside params was just an artificial mock property used in the unit test to verify what happens if a nested key shares the same name as a top-level column. In real Joomla articles, params of course stores options like show_title, show_author, link_titles, etc., while other JSON columns like images and urls store their own properties (image_intro, float_intro, urla).
The reason this matters in ContentHistory::getSha1() is the loop at line 138:
foreach ($object as $name => $value) {
if (\is_object($value)) {
// Go one level down for JSON column values
foreach ($value as $subName => $subValue) {
$object->$subName = \is_int($subValue) || \is_bool($subValue) || $subValue === null ? (string) $subValue : $subValue;
}
} ...
}Notice it assigns to $object->$subName instead of $value->$subName.
For every standard article with params (or images/metadata), this means:
- Every sub-field like
show_titleorimage_introgets copied onto the root object ($object->show_title), polluting the top level. - The property inside
$object->params->show_titleis never normalized to a string in the nested object where it belongs. - If any nested property in a custom component, module, or extension using Content History ever shared a key name with a column on
#__content, the root property would be silently overwritten during hash calculation.
Changing that line to $value->$subName ensures the sub-properties are normalized within their actual container ($object->params, $object->images), and decoupling $object ensures unset($object->$remove) doesn't strip modified_by/version from the caller's live table instance.
Hope that makes the intent and the test case clearer!
@brianteeman That was just a dummy property in the unit test data to test name collision with root fields, not a real Joomla article param.
The actual issue is at line 138 in ContentHistory.php where $object->$subName = ... writes to the root object instead of $value->$subName = .... So for any article with params or images, it was dumping all the nested properties (like show_title or image_intro) directly onto the root $object.
| Title |
|
||||||
| Labels |
Added:
bug
PR-6.2-dev
Removed: PR-5.4-dev |
||
As 5.4 has reached end of bugfix support and is now in security only support mode, I've allowed myself to rebase this PR here to 6.2-dev, where bug fixes should be made now. This worked cleanly, so the PR is still ok so far, it just needs 2 successful human tests.
Thanks @richard67 for rebasing it to 6.2-dev! Appreciate the help.
Why are we using json_encode() followed by json_decode() to copy the object? Would cloning the object and its nested objects not achieve the same result without the overhead of serialising everything to JSON?
I suspect that this is an AI generated PR
Please explain how you can set a title in the param