neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 221 forks source link

Dimensions - Changing structure of translated Content-Nodes causes them to disappear #3340

Open fdsis opened 3 years ago

fdsis commented 3 years ago

Description

After moving a Neos.Demo:Content.Columns.Two (with translated Neos.Demo:Content.Text in its columns) into the column of another Neos.Demo:Content.Columns.Two (in the main/fallback-dimension) the Neos.Demo:Content.Text-nodes disappear in the frontend ( in the other dimension). Tested using the demo-site with minimal changes.

Steps to Reproduce

Preparation

Changing the constraints of NodeTypes.Collection.Content.Column.yaml to enable testing with minimal demo-setup:

constraints:
    nodeTypes:
      '*': true

Changing the Dimensions in Settings.yaml to:

contentDimensions:
      language:
        label: 'Language'
        icon: 'icon-language'
        default: de
        defaultPreset: de
        presets:
          de:
            label: 'DE'
            values: [ 'de' ]
            uriSegment: ''
          en:
            label: 'EN'
            values: [ 'en', 'de' ]
            uriSegment: 'en'

(and then ./flow node:migrate 20150716212459, cache:flush, etc...)

Steps

  1. Create new Page called Test in language de
  2. create a Neos.Demo:Content.Columns.Two in main 2.1. put Neos.Demo:Content.Text in column0 with text de text 1 2.2 put Neos.Demo:Content.Text in column1 with text de text 2
  3. publish all changes to live
  4. switch language to 'en'
  5. translate texts to en text 1 and en text 2
  6. publish all changes to live
  7. switch language back to 'de'
  8. create a second Neos.Demo:Content.Columns.Two in main
  9. put the first Neos.Demo:Content.Columns.Two in column0 of the second Neos.Demo:Content.Columns.Two
  10. publish all changes to live
  11. open the page in the frontend
  12. switching language to 'en'

Expected behavior

The page should look like it does in the backend, with all nodes visible.

Actual behavior

The two Neos.Demo:Content.Text nodes are not rendered. The two Neos.Demo:Content.Columns.Two are rendered, though.

Affected Versions

Neos: 5.3, 7.1

kitsunet commented 3 years ago

This is coming from "intended" behavior to move content across dimensions together for reasons. This behvior is breaking in your case, although helpful for other cases. I got to tihnk about it and how a possible solution even could look like.

kitsunet commented 3 years ago

Note from me, I have been discussing this issue with and it's hard to fix universally due to potentially conflicting use cases demanding different behavior, BUT there is a solution build into Neos which boils down to configuring container elements (like a two column for example) with the

aggregate: true

option like so:

My.Package:CustomContainerElement:
    aggregate: true

Note: this is not something you should NOT do in all cases. By default Documents are configured like that. It will move childnodes around with the main node in all dimensions at the same time. Which might be undesired behavior, but it also prevents orphaned versions in other dimensions because the container was moved in a "parent" dimension.

I can draw out the scenario a bit more in detail at a later point. Just be aware that also the above solutoin might not be ideal but it will prevent the described problem.

bweinzierl commented 2 years ago

This Problem is still existent in latest Neos 7.3 releases. But now the workaround (setting the content elements to aggregate: true) is no longer working because there is some new integrity check logic preventing that. The code expects aggregates to be documents.

Here is a patch to make the workaround possible for Neos 7.x also:

Neos_Contentrepository_aggregate-check-workaround.patch.txt

@kitsunet There should really be a universal solution for this issue as it makes multidimension sites with fallbacks break by default. Without knowhing about this issue every multilang project with fallbacks will have a really bad editor experience sooner or later. Not sure how to help here but would be willing to contribute some time... maybe during / after neos con? There is a sprint. I will not be able to join physically but maybe remotely...

Sebobo commented 1 year ago

We ran into this issue now on neos.io. We really have to find a solution there. @bweinzierl I will test your patch and investigate further.

@kitsunet in which cases could the aggregate be an issue?

Sebobo commented 1 year ago

So from our debugging session, the solution seems to turn every Neos.Neos:ContentCollection into an aggregate. No further change or patch was necessary. This fixed all problems we had with a second language and its fallback behaviour, meaning we were able to move content in the primary language and the translated content of the second language followed inside deeply nested content elements. Before the translated content stayed at the old place and "got lost", like described in the issue. I'll create a patch and we have to decide on which version we can release it due to maybe some freakiness.

mhsdesign commented 1 year ago

the solution seems to turn every Neos.Neos:ContentCollection into an aggregate. No further change or patch was necessary.

due to maybe some freakiness.

i just talked to @nezaniel and it might be that certain indexers like the elasticsearch indexer use the aggregate state to determine if a node belongs to this "bucket" it might very well also be determined by the name Neos.Neos:Document in the case for elasticsearch, but turning every Neos.Neos:ContentCollection into an aggregate might be quite breaking.

edit: elasticsearch seems to use fulltext.isRoot instead: https://github.com/Flowpack/Flowpack.ElasticSearch.ContentRepositoryAdaptor/blob/d9c1b777b529605725dd993220b843dcad4251e1/Configuration/NodeTypes.Override.Document.yaml#L3-L4

Bernhard says it would be semantically correct, but we should put a warning in the release ^^

Paraldos commented 1 year ago

I tried to set 'aggregate: true' as instructed. After the change I get an error whenever I try to move a node. Even after removing the language dimensions (so only english remains) the error consists.

Bildschirmfoto 2023-08-10 um 10 27 52

Bildschirmfoto 2023-08-10 um 10 28 21

Bildschirmfoto 2023-08-10 um 10 28 52

Sebobo commented 1 year ago

@Paraldos afaik this can happen if content was already in two different places across the languages and the parents have different identifiers / paths. So if you remove Japanese from the configuration you might also have to remove its content. Should work via the ./flow node:migrate 20150716212459 migration.

I experienced the issue on Neos.io 3 times with content that was moved across languages two different parent containers. Those could only be fixed by making sure that the same parent exists in both languages and then move.

Do you really get this error with any node?

Paraldos commented 1 year ago

@Sebobo I rebuild the Situation SimonPaidla described here: https://github.com/neos/neos-development-collection/issues/943#issuecomment-1564151046 Complete with a new, empty page, and new text nodes to test the situation. Though I tried just now to move other, already existing nodes at a different page with the same result. I'll try now if ./flow node:migrate has any effect.

Paraldos commented 1 year ago

@Sebobo The migration did not help with the problem. The error is still there. However, after some more testing, I think I narrowed it down. Elements within a 'Stage' work fine, if 'aggregate: true' is set. Elements within a 'Two column content' trigger the mentioned error message. In my test I tried to move a Two column content with textfields into the col of another two column content (see img).

Bildschirmfoto 2023-08-10 um 13 23 52

Sebobo commented 1 year ago

@Paraldos thx for the detailed test. Now I can reproduce the issue in the Neos.Demo and will investigate.

Sebobo commented 1 year ago

@Paraldos It seems the two column element itself also has to be an aggregate, then it works. I'm not checking the code why this is and what the proper fix would be.

Sebobo commented 1 year ago

@Paraldos it's probably better to continue the discussion regarding the issue in #4436 as I work on the fix there and also found the reason for the problem you described.