spicywebau / craft-neo

A Matrix-like field type for Craft CMS that uses existing fields
Other
403 stars 63 forks source link

getSerializedFieldValues() on entry does not return deactivated blocks and breaks block structure #391

Open christianruhstaller opened 3 years ago

christianruhstaller commented 3 years ago

Description

For the SiteCopy (https://github.com/Goldinteractive/craft3-sitecopy) to enable copying from one site to another we use the getSerializedFieldValues method on the entry to get the serialized form of the field layout to save it to another entry.

The problem is that the method does not return deactivated blocks (It just skips them). For the copying this isn't nice but not a catastrophic error, except if you have a multi level structure and only the parent is deactivated but not the children then what happens is that the method getSerializedFieldValues returns the children but not the parent.

If we use the result of the method and save it to another entry we lose the parent block and the neo structure is broken/wrong.

Example with an accordion. We have a accordion wrapper which has the entries as its children: Level 1: Accordion Level 2 (Children): Accordion entry

Entry: Neo field:

After using the result from getSerializedFieldValues the structure will be:

We lost the parent block "Accordion" and the funny thing is the child blocks are not on the same level anymore. If you open the entry it will take the first child and use it as a parent.

From the returned data from getSerializedFieldValues you would expect this:

But that does not happen.

getSerializedFieldValues should always return all the blocks no matter the status of the corresponding block and especially it should not create a bogus structure.

Any inputs/opinions on this? I hope my report is comprehensible. :D

Other information

christianruhstaller commented 3 years ago

Addendum: It seems that the matrix has the same behavior. It seems that this is a bigger issue and maybe even the wrong method to handle copying.

I think it would be okay if deactivated blocks do not returned at all but at least the integrity of the structure should be maintained.

ttempleton commented 3 years ago

I explained in a bit more detail in #372 how the behaviour of a Neo field's serializeValue() method is consistent with Matrix and structure section entry queries, and why serializeValue()'s return value should therefore not be considered reliable structure data if only enabled blocks are included. That said, I did also discuss whether enabled blocks should be excluded when querying for enabled blocks if they have a disabled ancestor; at this point (for consistency with structure section entry queries) I'm inclined to leave it as it is, but it's worth considering.

I think disabled blocks should be returned by getSerializedFieldValues() -- or at least have the option to set that -- and that the ideal solution to this would be if we could set anyStatus() on the block query if getSerializedFieldValues() is involved, but that doesn't seem possible at the moment as far as I can tell.

lukasNo1 commented 3 years ago

As a workaround, in the SiteCopy plugin we override the getSerializedFieldValues() function to return both enabled and disabled elements for now. https://github.com/Goldinteractive/craft3-sitecopy/commit/91334a1197d99965cffff9fddbe7c27a9b47f24e#diff-85a66352a4b6e09286584934d796f53eR284