search

craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.22k stars 626 forks source link

[5.x]: Entry types' field layout changes aren't saved, if the only thing that changed were the positions of layout elements #15154

Closed mmikkel closed 3 months ago

mmikkel commented 3 months ago

What happened?

Description

Changes to an entry type's field layout aren't actually saved, if all that was changed was field layout elements' positions:

https://github.com/craftcms/cms/assets/298510/65df2902-27fa-4ba1-beab-1b290508c535

If other things are changed in addition to the new layout elements' position (such as adding or removing an element, changing elements' settings etc) the new layout elements positions are saved as expected.

Steps to reproduce

  1. In an entry type's edit page, move a field layout element (make no other changes)
  2. Save the entry type
  3. Confirm that the moved element is still in its previous position

Expected behavior

Saving an entry type field layout should persist layout elements' positions, even if nothing else changed.

Actual behavior

Layout elements' positions are only saved if something else was changed in the layout too.

Craft CMS version

5.2.0-beta.3

PHP version

No response

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

None

brandonkelly commented 3 months ago

I’m not able to reproduce that. Are you getting any JS errors?

mmikkel commented 3 months ago

No JS errors, and I went back and tested w/ a clean 5.2.0-beta.3 install in a few different browsers (Chrome, Edge, Safari) – same thing.

I'm not able to reproduce at all on 5.1.9 though, so seems isolated to the 5.2 beta.

brandonkelly commented 3 months ago

I tested on 5.1 and 5.2, but couldn’t reproduce either way. Even tried with a field layout that only contains two instances of the same field.

https://github.com/craftcms/cms/assets/47792/021f9edf-e9e8-4e02-a946-8f144c5f445e

Tested in Firefox, Edge, and Safari, as well as on another 5.2 install on Brad’s computer.

Did you use the craftcms/craft starter project for the fresh install, or do you have your own starter project?

mmikkel commented 3 months ago

That's really strange.

The fresh install is a completely stock craftcms/craft starter install, running on DDEV v1.23.1 and installed as per the official docs' Quick Start section.

Literally all I did to reproduce was to update the site to 5.2.0-beta.3 immediately after installation (5.2.0-beta.3 as 5.1.9), and then I added a single entry type with two PlainText fields.

A few other things I tried to see if it would affect anything:

mmikkel commented 3 months ago

Here's something else that could be relevant:

I tried downgrading the fresh install from 5.2.0-beta.3 to 5.1.9 and was surprised to now being able to reproduce on that version as well. On a whim, I renamed the entry type, after which the behavior completely went away on 5.1.

I then upgraded the site back to 5.2.0-beta.3, and the behavior was immediately back. I did this two times to make sure, and it was the same each time 🤷

i-just commented 3 months ago

I can replicate it. Looking into it. I’ll update this issue once I know more.

brandonkelly commented 3 months ago

Sorry! I was testing on installs with the 5.2 branch checked out, but I forgot to run composer update to pull in the Yii 2.0.50 update.

@i-just tracked this down to a Yii bug related to determining whether JSON columns have changed, which was exacerbated by yiisoft/yii2#19915 added in 2.0.49.

I’ve reported the bug (yiisoft/yii2#20191) and submitted a PR to fix (yiisoft/yii2#20192). In the meantime, the least-hacky way I could find to fix this in Craft was to change the nature of ArrayHelper::recursiveSort() so it skips non-associative arrays altogether. Not great but there’s no other usages of this method besides by ActiveRecord, at least in Craft/Yii. Hopefully it’s a short-lived fix.

mmikkel commented 3 months ago

Wow, that went a lot deeper than I thought it would 😅 Appreciate the fix Brandon & @i-just 🙌

brandonkelly commented 3 months ago

Craft 5.2.0-beta.4 is out now with the temporary fix.

mmikkel commented 3 months ago

@brandonkelly Unfortunately, it looks like the temporary fix is triggering the following fatal error if the config/general.php file is using the multi-environment (i.e. not fluent) config syntax:

Fatal error: Cannot declare class yii\helpers\ArrayHelper, because the name is already in use in /var/www/html/vendor/craftcms/cms/lib/yii2/helpers/ArrayHelper.php on line 13

Plugin config files using multi-environment syntax seem fine, it's specifically a multi-environment-formatted config/general.php file that seems to trigger the error.

brandonkelly commented 3 months ago

Doh, thanks. 5.2.0-beta.5 is out with a fix for that.