Project

General

Profile

Actions

Bug #90939

open

DataHandler: Existing pages are not moved into newly created pages

Added by Jonas Eberle about 4 years ago. Updated almost 3 years ago.

Status:
New
Priority:
Should have
Assignee:
-
Category:
DataHandler aka TCEmain
Target version:
-
Start date:
2020-04-03
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
10
PHP Version:
Tags:
Documentation
Complexity:
Is Regression:
No
Sprint Focus:

Description

I suspected from the documentation https://docs.typo3.org/m/typo3/reference-coreapi/master/en-us/ApiOverview/Typo3CoreEngine/Database/Index.html that this should work:

$data = [
    'pages' => [ 
        'NEW_IT' => [
            'pid' => 1,
            'hidden' => false,
            'title' => 'IT',
        ],
        591 => [
            // this is not set
            'pid' => 'NEW_IT', 
            // but this is set
            'title' => 'I am a child of IT', 
        ],
    ]
];

Yet it creates the new page but only sets the property 'title' on page 591. Page 591's 'pid' is not changed.

@Stefan Bürk has done a tremendous job in verifying that from TYPO3 8-10 and also finding that the DataHandler explicitly disallows that (https://stackoverflow.com/a/61000253/2819581)

The question is if either
a) documentation should be amended
b) DataHandler can be fixed

Actions #1

Updated by Benni Mack about 4 years ago

  • Status changed from New to Needs Feedback

The example is wrong - The one should be a datamap (create page) the other should be a command map (move) I assume - right?

Actions #2

Updated by Jonas Eberle about 4 years ago

I tried with this as well:

$data = [
    'pages' => [ 
        'NEW_IT' => [
            'pid' => 1,
            'hidden' => false,
            'title' => 'IT',
        ],
        591 => [
            'move' => 'NEW_IT', 
        ],
    ]
];

Yet this is not in a command map.

Do you mean it is just not documented that 'pid' is not supported in data map?

Actions #3

Updated by Stefan Bürk about 4 years ago

@Jonas Eberle

That construct is invalid:

$data = [
    'pages' => [ 
        'NEW_IT' => [
            'pid' => 1,
            'hidden' => false,
            'title' => 'IT',
        ],
        591 => [
            'move' => 'NEW_IT', 
        ],
    ]
];

That mixed $data and $cmd structure in on go. That is not supported. In that case, it tries to update 591 and set the database field 'move' to 'NEW_IT'.


So, what could be the use-case to came up with the try to move the record in a dataMap call:
As I read the stackoverflow / slack notes from Jonas, I remebered some huge migration stuff in the last years in minimum one project.
The we had to change a lot in the page structure. That was moving pages / records and updating further fields in the records. Even mixed with creating new ones.
Because it was not working, because of how datahandler works, it was a huge pane to manage and doing different cylies, managing/retrieving the realUids and so on.

And what is not clear, but a developer could thing (from a record based view) - if i can update fields of a database record, why not update the pid ( => move ) ?


Benni Mack wrote:

The example is wrong - The one should be a datamap (create page) the other should be a command map (move) I assume - right?

No - it is not wrong. The documentation is.

If you read the documentation and examples, it first came up with examples for new ones, where the 'pid' field is used ( with NEW<id> ) for creating new records (pages).
Then, there is a example for an record update (example without pid). There is nowhere noted, that pid is only evaluated/used for creation, not for updating. (and silently ommited there). (As other fields to).

So, it is not expressivly allowed for updating records using the dataMap datahandler part, but it is not stated that it is not allowed / working.
The further thing is - it is silently discarded. You get no "feedback". No errorlog entries, if datahandler errorlogging is enabled.

So no - no error, so it should be allowed, beside it is not working.

Yes - the example is wrong - but the documentation also
If you are a new developer, or new/unexperienced with the datahandler, where would you now that this is not expressivly disallowed.

Changing pid is a move. Moving and updating in one go is not allowed.
If it is wrong to update the pid through the dataMap part, it should be considered as error and throw/nothing an error - or not ?

Basicly it could be the "better" way to leave it - adding a kind of "move & update" to the dataMap part could introduce complexity you want to ommit.

------------------
Also I had myself this usecase in the past, and now Jonas, I could live that there "is no need to allow a pid field record update move" behaviour.

But at least two things should be done:

- updating the documentation (see below for some thought what should be added)
- add/throw error if pid (or other suppressed fields) is tried to update in record-update-mode

Just to make other would not waste time in something which sound logical, but is not supported.

The other point - would it be so bad, to allow the moving in the dataMap with record field updates ? Or could we say that it is a duplicate, but a valid usecase - and worth to try to implement it ?


Updating the documentation:

- clearify the two modes for the dataMap part. Eventually name them in some ways ( NEW<id> => data-record-create, RealIntUid => data-record-update )
- in the "describe keywords in syntax" table add a note to the value field, that some fields are mainly dropped/ignorend in data-record-update mode, but have special meanings in data-record-create mode.
- split examples more prominent for the two modes.
- eventually add a note you need different cyles of datahandler calls/processes instead doing things in one run.

Actions #4

Updated by Jonas Eberle about 4 years ago

To put it simple: I did not expect from the documentation that I am ONLY allowed to change pid with the 'move' command.

I cannot see how the first example ('pid' => 'NEW_IT') is invalid. The second one is, of course.

Actions #5

Updated by Gerrit Code Review about 4 years ago

  • Status changed from Needs Feedback 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/+/64063

Actions #6

Updated by Gerrit Code Review about 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/c/Packages/TYPO3.CMS/+/64063

Actions #7

Updated by Gerrit Code Review about 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/c/Packages/TYPO3.CMS/+/64063

Actions #8

Updated by Gerrit Code Review about 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/c/Packages/TYPO3.CMS/+/64063

Actions #9

Updated by Christian Kuhn almost 3 years ago

  • Status changed from Under Review to New
Actions

Also available in: Atom PDF