Bug #59901
closedMove records in list module kills sys_language_uid
100%
Description
Hey there.
I discovered a very nasty thing crashing lots of tt_contents silently.
I call it a "Must have" because it really messes everything up. When you drop a single new tt_content at the end of backend list view and push it all the way up to the very first list postion in backend list view, nearly every tt_content in between gets touched and looses its language.
Example situation:¶
- tt_content:1, sys_language_uid:1
- tt_content:2, sys_language_uid:0
- tt_content:3, sys_langugae_uid:1, l18n_parent:2
Open list module in "Localization View", which allows to move tt_content:1 one step down or tt_content:2 one step up.
Example action:¶
Now click on tt_content:2 to move it above tt_content:1.
Result:
tt_content:2 gets repositioned, just as expected.
tt_content:1 gets updated by DataHandling::moveRecord() -> GridElements\DataHandler\ProcessCmdmap::execute_processCmdmap() -> DataHandler::copyRecord()
I don't really get what this hook thingy is meant to do.
But in the end tt_content:1:sys_language_uid is updated from 1 to 0 because tt_content:2:sys_language_uid is 0.
Example action:¶
Revert to example situation, of course.
Now click on tt_content:1 to move it below tt_content:2.
Result:
tt_content:1 gets repositioned, just as expected.
tt_content:2 gets updated by the very same hook queue.
In the end, tt_content:2:sys_language_uid is updated from 0 to 1 because tt_content:1:sys_language_uid is 1.
Fix:¶
See patch attached.
I would suggest to drop "sys_language_uid" from "copyAfterDuplFields" when calling the hook manually. That shouldn't do much of a difference because I cannot imagine any situation where the sys_language_uid should by updated.
Updated by Jo Hasenau over 10 years ago
copyRecord() must use the copyAfterDuplFields and especially sys_language_uid to make sure that copying after an existing elements gets the necessary things like container, column, language.
But actually moving a record should not call anything related to copyRecord(), so I guess, this is the bug here.
Updated by Stephan Schuler over 10 years ago
Jep, that's what I saw right after pushing the ticket, and that's the reason for not having a patch applied.
I need to dig a bit deeper into my case here and see why my situation gets treated as copy command. I'll reply back as soon as I know what causes this, either by updating the description or by dropping this ticket.
Updated by Stephan Schuler over 10 years ago
Here's what happens in detail:
- Move a tt_content
- GridElementsTeam\Gridelements\Hooks\DataHandler::moveRecord gets called
- GridElementsTeam\Gridelements\DataHandler\MoveRecord::execute_moveRecord gets to decide which move post processing to do
- That's where copyAfterDuplFields is manipulated,
- and that's where I would adjust copyAfterDuplFields to simply not use sys_language_uid here, btw
- It's the "else if($cmd['tt_content'][$uid]['move'])" what matches in your MoveRecord class
- So TceMain->moveRecord_raw() is triggered
- Since it' sorting by not pointing to a page id ($destPid>=0) but to a record ($destPid<0), \TYPO3\CMS\Core\DataHandling\DataHandler::moveRecord_raw() calls $this->fixCopyAfterDuplFields() at line 3869 (TYPO3 6.2.2)
- And \TYPO3\CMS\Core\DataHandling\DataHandler::fixCopyAfterDuplFields is where sys_language_uid gets updated
I do get the reason why that's the way it works: To at least adjust e.g. colpos if you drag/drop a certain content element from one column to another element in another column. There it's obvious that coplos needs to be adjusted as well.
But when you do not use drag/drop page view mechanisms but list module arrows, the up/down buttons should not just adjust language and colpos.
And ... updating the language is completely unexpected to me.
When updating sorting in the list module, neither language nor colpos should be updated. I don't expect a single tt_content to move from one column to another and one langauge to another when using the list module. When changing relative positions of two records in the list module where both of them are not in any relation (like two contents in completely different columns), I'd rather have that move operation not result in any frontend changes then in some magic.
When updating sorting or colpos in the page module it's kind of different, but even more restrictive. If I drag a tt_content with a certain language, I'd rather be not able at all to drop it to a completely different language container (technical: just skip the drop zone) then being able to drop it to a different language container hat have it automatically switched to that particular language.
I'm not completely sure what's to be done here. I have a patch, but it only avoids core mechanisms in a very specific situation.
I'm going to update the test case in my first post because I missed some correlations.
Regards,
Stephan.
Updated by Stephan Schuler over 10 years ago
Well, I cannot update my first post. So here's the complete situation:
- tt_content:1, sys_language:uid:2
- tt_content:2, sys_language_uid:1
- tt_content:3, sys_language_uid:0
- tt_content:4, sys_langugae_uid:3, l18n_parent:2
No move tt_content:3 one step above.
It targets tt_content:1 and therefore inherits sys_language_uid:2 from it.
- tt_content:3 with sys_language_uid:2
- tt_content:4 with sys_language_uid:1 and l18n_parent:2
- which is an invalid configuration because l18n_parent doesn't target a L=0 or L=-1 record
The reason for that, as explained, is the assumptin that whenever moving a tt_content from one position to another the target position should influence both, colpos and langauge of the record to be moved. That might be right when using drag/drop in page modules, but it's just not true when moving objects in the list module.
Updated by Jo Hasenau over 10 years ago
As far as I can see, the only records you can move up and down in the list module are those, that are not children of a Grid Container.
The rest has not got any up and down arrows, so the problem won't happen there.
The technical problem behind this bug is: The core knows exactly two "positions" while moving, inserting or copying records
1. After a record
2. At the first place of a column
So moving after a record with a different language should be a problem of the core as well.
Now the question is, what is the behaviour when you uninstall gridelements and try the same stuff?
Updated by Stephan Schuler over 10 years ago
Having grid children isn't the real point here, I'm just talking about plain non-grid records.
But I'm reporting here because the situation is only triggered because of the post move hook of gridelements which is meant to keep track of grid relations.
- Lines 1, 2 and 3 have arrows, because they are no translation children.
- Line 1 and 2 are "native foreign language records", described by "(sys_language_uid > 0 AND l18n_parent = 0)".
- Line 3 is a native default language record,
- Line 4 is its translation.
In this situation, the default TYPO3 list module shows three move arrows: For line 1, line 2 and line 3.
Line 4 shows no move arrow, but that's doesn't affect the situation and is completely expected.
You might have a look at \GridElementsTeam\Gridelements\DataHandler\ProcessCmdmap::execute_processCmdmap().
Maybe it's enough to restrict this particular hook to records where "tx_gridelements_container!=0"?
Unfortunately I'm a bit bussy at the moment. I'll keep investigating and report back with results, but probably not today.
Updated by Jo Hasenau over 10 years ago
- Status changed from Under Review to Needs Feedback
- Priority changed from Must have to Could have
After uninstalling Gridelements the behaviour is exactly the same, due to the following tt_content default setting of the core:
'copyAfterDuplFields' => 'colPos,sys_language_uid',
So if you think, this should be changed, you should discuss this with some people in the core tracker.
I will move this issue there.
Updated by Jo Hasenau over 10 years ago
Checked the core code and "moveRecord_raw" is always the final method that moves a record unless it has been moved by a hook already.
Gridelements is using a hook and setting a variable to prevent that, but internally we are of course using the same method, since moving should be handled almost identical to the core with some additional stuff for nested structures.
So "fixCopyAfterDuplFields" will always be used for a move action with our without Gridelements.
Updated by Christian Hernmarck over 10 years ago
Hi
I also have the same problem: when moving content up/down the sys_language_uid (and also colPos) often changes.
I use TYPO3 4.5 and the infos here helped me to dig deeper into the thing.
As far as I can see:
- fixCopyAfterDuplFields sets the fields which are listed id $TCA[$table]['ctrl']['copyAfterDuplFields'] to the same value as the content element "above"... (default: colPos and sys_language_uid)
- this may be ok if you move an element by cut it and paste it somewhere "after another element" - but it's not usefull if you move the element up or down (in the list or page view) - so far the same as in comment 1
With the latest update to 4.5.35 the "colPos-changing when moving" (issues 48939 and 49055) has been "solved" by hiding the "move-up" and "move-down" buttons in the page view when not needed... but the problem still exists in the list view.
So I'd say the solution would be to tell "moveRecord_raw" if it's a "move-up"/"move-down" action or a (cut&)paste-after... command...
At this point my knowledge of the internals are limited... where to put the changes (a boolean flag with a default would be IMHO sufficient).
Hope this helps - at least to push this issue... to "should have"....
Updated by Christian Hernmarck over 10 years ago
A proposal...
if my "solution" above is good, then I'd change theese lines...
in class.t3lib_clipboard.php, line 958 (in 4.5.35):
old:
$mode = $this->currentMode() == 'copy' ? 'copy' : 'move';
new:
$mode = $this->currentMode() == 'copy' ? 'copy' : 'moveCB'; #moveCB = move (cut&paste) from ClipBoard
and in class.t3lib_tcemain.php, several lines (all from 4.5.35):
from line 2660:
old:
case 'move': $this->moveRecord($table, $id, $value); break;
new:
case 'move': case 'moveCB': $this->moveRecord($table, $id, $value, $command == 'moveCB'); break;
and later orig line: 3495:
old:
function moveRecord($table, $uid, $destPid) {
new:
function moveRecord($table, $uid, $destPid, $moveCB=true) {
and 3554:
old:
$this->moveRecord_raw($table, $uid, $destPid);
new:
$this->moveRecord_raw($table, $uid, $destPid, $moveCB);
and 3580:
old:
function moveRecord_raw($table, $uid, $destPid) {
new:
function moveRecord_raw($table, $uid, $destPid, $moveCB = true) {
and then, finally the two blocks in 3649 and 3702:
old:
if ($origDestPid < 0) { $this->fixCopyAfterDuplFields($table, $uid, abs($origDestPid), 1);
new:
if (($origDestPid < 0) and $moveCB) { $this->fixCopyAfterDuplFields($table, $uid, abs($origDestPid), 1);
What do you think guys?
Just try to pass the information about "it's a cut&paste" to the actual function which does the movement...
/Christian
Updated by Christian Hernmarck over 10 years ago
a better way... without changing the command...
This one checks if $SOBE->CN is an array just before calling fixCopyAfterDuplFields - if $SOBE->CN is empty then it's not a clipboard action...
(see tce_db.php).
only class.t3lib_tcemain.php - as patch...
--- class.t3lib_tcemain.php_o 2014-07-08 14:45:00.000000000 +0200 +++ class.t3lib_tcemain.php 2014-07-11 23:01:46.000000000 +0200 @@ -3578,7 +3578,7 @@ * @see moveRecord() */ function moveRecord_raw($table, $uid, $destPid) { - global $TCA, $TYPO3_CONF_VARS; + global $TCA, $TYPO3_CONF_VARS, $SOBE; $sortRow = $TCA[$table]['ctrl']['sortby']; $origDestPid = $destPid; @@ -3646,7 +3646,7 @@ $this->clear_cache($table, $uid); // clear cache after moving $this->fixUniqueInPid($table, $uid); // fixCopyAfterDuplFields - if ($origDestPid < 0) { + if (($origDestPid < 0) && is_array($SOBE->CB)) { $this->fixCopyAfterDuplFields($table, $uid, abs($origDestPid), 1); } // origDestPid is retrieve before it may possibly be converted to resolvePid if the table is not sorted anyway. In this way, copying records to after another records which are not sorted still lets you use this function in order to copy fields from the one before. } else { @@ -3699,7 +3699,7 @@ $this->fixUniqueInPid($table, $uid); // fixCopyAfterDuplFields - if ($origDestPid < 0) { + if (($origDestPid < 0) && is_array($SOBE->CB)) { $this->fixCopyAfterDuplFields($table, $uid, abs($origDestPid), 1); } } else {
This works very well for me - and I think it really should not have side effects.
Updated by Alexander Opitz about 10 years ago
- Is Regression set to No
Hi,
does the problem still exists within newer versions of TYPO3 CMS (6.2.7)?
Updated by René Fritz about 10 years ago
6.2.6 is still affected by this bug.
The patch from Christian Hernmarck works but this is just a workaround I would say. Tcemain does not know anything about modules (which $SOBE is).
Updated by Alexander Opitz over 9 years ago
- Category set to DataHandler aka TCEmain
- Status changed from Needs Feedback to New
Updated by Jo Hasenau over 9 years ago
- TYPO3 Version changed from 6.2 to 7
Still broken in TYPO3 versions up to current master. IMHO the problem is sys_language_uid being listed in copyAfterDuplFields, which is used not just by copy actions but by move actions as well.
Updated by Riccardo De Contardi almost 7 years ago
- Related to Bug #72988: losing Localization when moving elements (Typo 7.6.2) added
Updated by Tymoteusz Motylewski over 6 years ago
- Related to Bug #16845: Incorrect values of colPos, sys_language_uid and sorting after moving records in web > list added
Updated by Georg Ringer almost 5 years ago
- Has duplicate Bug #90691: Moving records in list module changes their language added
Updated by Christian Eßl almost 5 years ago
- Related to Bug #39798: Language and colpos changes on re-ordering of Content Elements added
Updated by Oliver Hader almost 5 years ago
- Status changed from New to Accepted
The behaviour of calling DataHandler::fixCopyAfterDuplFields
when moving records stems from very early days in TYPO3:
https://github.com/TYPO3/TYPO3.CMS/commit/dca0d260fdd9b8dd6fded57baebfd600519ffbfb#diff-25c5bd0227c9efb7aec0fc0a77f30fb9R2399
Basically "moving a record" in TYPO3 backend can be:
- move to a different page (positive
pid
value, being inserted at the top -sorting
field of record is adjusted accordingly) - move after an existing record (which could be on a different page as well - negative
pid
, fetch value havingabs(pid)
and that the resolvedpid
-sorting
field is adjusted accordingly to be after existing record)
There is no task like "move to another colPos
", this is basically an update
process in general.
For the use-case "move after an existing record in another colPos
" to would have to be
- a plain move (resorting) and moving record after existing record
- dedicated update call for
colPos
(must be given explicitly) DataHandler::process_cmdmap
supportspaste
andupdate
which actually allow to define commands "copy + update" or "move + update, see https://docs.typo3.org/m/typo3/reference-coreapi/master/en-us/ApiOverview/Typo3CoreEngine/Database/Index.html#command-keywords-and-values for details
Conclusion:
- check/verify which parts of the core (backend, page module, form engine) probably could be using this (search for
colPos
as indicator) - adjust those commands to be "copy + update" or "move + update" (as mentioned in previous section)
- remove invocation of
DataHandler::fixCopyAfterDuplFields
in "move" processes (and keep it for "copy")
Updated by Oliver Hader almost 5 years ago
- Priority changed from Could have to Should have
- Complexity set to medium
Updated by Gerrit Code Review almost 5 years ago
- Status changed from Accepted 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/c/Packages/TYPO3.CMS/+/63646
Updated by Gerrit Code Review almost 5 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/c/Packages/TYPO3.CMS/+/63646
Updated by Gerrit Code Review almost 5 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/c/Packages/TYPO3.CMS/+/63646
Updated by Gerrit Code Review almost 5 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/c/Packages/TYPO3.CMS/+/63646
Updated by Gerrit Code Review almost 5 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/c/Packages/TYPO3.CMS/+/63646
Updated by Georg Ringer almost 5 years ago
- Status changed from Under Review to Closed
duplicate of #39798, closing it now, patch is already pending
Updated by Gerrit Code Review almost 5 years ago
- Status changed from Closed to Under Review
Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64137
Updated by Christian Eßl almost 5 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset acff9da490244cd405a4ae12eb7f034a3b39cfb6.
Updated by Benni Mack almost 5 years ago
- Status changed from Resolved to Closed