search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
722 stars 821 forks source link

DiffField breaks when applied to TreeMultiselectField #9843

Closed Cheddam closed 3 years ago

Cheddam commented 3 years ago

Affected Version

CMS Recipe 4.7.0 and later

Description

The changes introduced in #9604 added custom behaviour for the setValue() method on TreeMultiselectField. Before, it would directly set whatever $value was passed (generally null or a scalar value), but now it attempts to set the value to a relationship matching the field name, which can result in a ManyManyList being set as the value.

This causes issues when the field is transformed into a DiffField. When generating the form schema for this field, it tries to pull the value of the original TreeMultiselectField and pass it to the Diff::compareHTML() method, which only supports scalar / array values.

A primary example of this problem is that if a TreeMultiselectField is added to a DataObject that has a History view, the comparison feature of the History view will cease to function. This directly impacts the CWP recipe, which adds a TreeMultiselectField to all pagetypes to manage Taxonomy Terms.

(It's worth noting that AFAICT, DiffField's handling of TreeMultiselectField was broken before this, too - it just resulted an empty diff, rather than throwing an exception.)

Steps to Reproduce

  1. Create a project with Composer using the cwp/cwp-installer recipe
  2. Load the Pages UI, and select a Page
  3. Open the History tab, select a version, and click Compare
  4. Select another version, and observe that the comparison never loads (the API request throws a 500.)

Possible solutions

I believe the change in behaviour introduced by #9604 still makes sense, so to me the best course of action would be stopping DiffField from attempting to diff unsupported values, or possibly transforming said values before diffing them.

brynwhyman commented 3 years ago

Gah, looks like this was already raised https://github.com/silverstripe/silverstripe-framework/issues/9839

That issue has some additional detail on the 'CWP specifics' too:

install CWP (this is not requried - any manymany or manyone relationship is needed to break it. This is a module that adds Taxonomy to all page types which will break the diff view)

@Cheddam I'll leave it to you which one you want to keep open.

Cheddam commented 3 years ago

Fixed in silverstripe/silverstripe-versioned-admin#197.

michalkleiner commented 3 years ago

The reference to the PR in which it was fixed in your comment @Cheddam resolves to this repo and it should be the versioned-admin repo instead, so I was like... 2012 PR? :-D

Cheddam commented 3 years ago

Whoops, fixed @michalkleiner 😅