Bug #69334

Rollback on content element doesn't restore deleted File Reference

Added by Nicolas Ries about 4 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2015-08-26
Due date:
% Done:

0%

TYPO3 Version:
6.2
PHP Version:
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

Description

Deleting an image inside of a content element and then using the History / Undo function on the content element to restore the image doesn't bring back the deleted file reference. Instead it takes a completely different record from the sys_file_reference table that wasn't deleted and points the uid_foreign field to the content element, practically stealing the file reference from another content element.

Steps to reproduce the issue:
  • create new "Images" element
  • add a picture to it and save
  • delete the picture and save
  • use History / Undo on the "Images" element and undo the last step
  • the element will now have a completely different image to before while the deleted file reference is still deleted

Undeleting the file reference by doing History / Undo on the page the content element and file reference are on works correctly and you can bring back the deleted image that way but I think that's a very unintuitive for most users.

cattura.png View (55.2 KB) Riccardo De Contardi, 2015-10-16 09:46


Related issues

Related to Grid Elements (former official tracker) - now moved to Gitlab! - Bug #75212: History/Undo changes foreign content elements on other pages Closed 2016-03-21
Related to TYPO3 Core - Bug #71245: Undelete does not restore inline records New 2015-11-02

History

#1 Updated by Riccardo De Contardi almost 4 years ago

It is still present on 6.2.15; on 7.6-dev (latest master), the steps described above lead to this error:

No file reference (sys_file_reference) was found for given UID: "1" 

see attached screenshot

#2 Updated by Erik Frister almost 4 years ago

Removing and adding a new image in the same step is also a problem. That action doesn't even show a modification in the history.

#3 Updated by Gerrit Code Review almost 4 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#4 Updated by Erik Frister almost 4 years ago

Notes about the patch.

The patch provides a solution for both issues, namely:

1. Looking at a record's history does not show the history of related records, e.g. images
2. Rolling back changes that were made in an "inline" column (e.g. "image" of tt_content) lead to an exception.

Proposed Solution for each:
1. Updated ReferecenIndex class to optionally return deleted relations as well. Updated RecordHistory to use this feature and list all changes for relations, as well as their "insert" and "delete" actions, to enable a full rollback of all related changes.
2. Fixed the "removeFilefields" method of RecordHistory to actually unset file fields. Be aware this just makes sure that no exception is thrown as no invalid values are passed to the data map. Fixing the underlying issue of being able to roll back file fields is something that I did not address as that is non-trivial.

Details regarding issue 1:
The general solution would be to display the history records of a record AND it's related records (e.g. the "image" sys_file_references alongside the tt_content element that has them), to make sure changes within an element's IRRE relations can be viewed when looking at an elements history.

  • tried to use the regular ref_index (TYPO3\CMS\Core\Database\ReferenceIndex), but it's currently impossible to include deleted(!) db records in the relations that are being returned. That would make it impossible to roll back deletions
  • working with the TYPO3\CMS\Core\Database\RelationHandler directly duplicates a lot of the code that is already in the \ReferenceIndex class

Thus the proposed solution is to add an additional property to \ReferenceIndex indicating if deleted records should be returned as well. Setting this flag to TRUE would yield all necessary relations when getting related elements. Getting those records' history as well gives us the desired result.

The attached patch does this.

Details regarding issue 2:
The problem currently is that RecordHistory creates a datamap where the value for e.g. the "image" field (type "inline") is the COUNT of the images, not the sys_file_reference uids.
Example:
Assume a user adds two references with uid 80 and uid 81, the datamap should look like:
$datamap['tt_content']['NEWabc']['image'] = '80,81';
BUT the RecordHistory does the following:
$datamap['tt_content']['NEWabc']['image'] = '2';
where 2 is the COUNT of the files added. This is because the sys_history entry uses the FINAL value of the datamap as the diff. But the final value is the result of a transformation during "process_datamap", i.e. the '80,81' - during process_datamap - is replaced by the count, which is 2. However, the input(!) datamap HAS to list the sys_file_reference uids, not the count. Thus, when using "2" during the rollback, the process_datamap inteprets the 2 as being the sys_file_reference that is supposed to be used, which - if that doesn't exist - throws the exception we see.
The solution is not straight forward for me. As there was a method to exclude certain fields from rollbacks, I simply fixed this (it wasnt working properly) and extended it to also unset inline fields. In reality, this translates to being able to roll back "inline" fields as the actual relations are rolled back correctly, only the parent record has an incorrect state. Whether or not this is an actual problem is not clear to me. I'd assume it is during the language translation overlay processes, but I didn't investigate further. If anyone has a hint as to how this could be solved completely, please leave a comment or update my patch.

#5 Updated by Gerrit Code Review almost 4 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#6 Updated by Gerrit Code Review almost 4 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#7 Updated by Gerrit Code Review almost 4 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#8 Updated by Gerrit Code Review over 3 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#9 Updated by Gerrit Code Review over 3 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#10 Updated by Gerrit Code Review over 3 years ago

Patch set 7 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#11 Updated by Jo Hasenau over 3 years ago

Just added another issue from the gridelements tracker that seems to be based on the same problem: Number of records is taken as a UID value.

#12 Updated by Gerrit Code Review about 3 years ago

Patch set 8 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#13 Updated by Gerrit Code Review almost 3 years ago

Patch set 9 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#14 Updated by Gerrit Code Review almost 3 years ago

Patch set 10 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/44576

#15 Updated by Mathias Schreiber over 1 year ago

  • Status changed from Under Review to Closed

closing this as dupe of #71245

We'll stick to the never issue because there's active work involved.

#16 Updated by Mathias Schreiber over 1 year ago

  • Related to Bug #71245: Undelete does not restore inline records added

Also available in: Atom PDF