[4.x]: When removing money entry from entry type data stays saved as string

[michtio]

What happened?

Description

When a money field has existed on multiple entry types, but is later removed from an entry type. The field value stays in the database content table. This value is now handled as string, which throws an error applying the |money filter.

Steps to reproduce

1. Create 2 entry types on a section
2. Create a money field and add it to both entry types
3. Enter data in the entry in the money field on both entry types
4. Remove the money field from 1 entry type
5. The template will now error out if we try to parse an overview of all the entries in said section

Expected behavior

When a money field is removed from an entry type, also remove all associated data of the money field in the database, so we can generate an overview without throwing any error. Or keep the data in the database but don't parse the field as it's disassociated with the field layout.

Actual behavior

When adding entry.moneyField to loop over all entries (without checking the entry types, e.g. for an overview) the entry types that used to have a money field are now throwing the following error:

craft\web\twig\Extension::moneyFilter(): Argument #1 ($money) must be of type ?Money\Money, string given...

Craft CMS version

4.3.6.1

PHP version

8.0

Additional info

Trying to find a workaround is a bit bulky, using the resave command for bulk editing isn't a solution since the field doesn't exist anymore. The only solution is to add/create the field again to the entry type. Save everything with :empty: deploy, do this on staging / production, to remove the field again and deploy again. This is quite cumbersome.

docker exec -it hdac-php-1 su-exec www-data php craft \
                                resave/entries --set semesterlyPrice --to :empty: --type==team --section=classes
Resaving 9 entries ...
    - [1/9] Resaving Chaos (3216) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    - [2/9] Resaving Team Infinity (3191) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    - [3/9] Resaving Team Felicity (3205) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    - [4/9] Resaving Team Diva Diversity (3180) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    - [5/9] Resaving Anthro-pos (3167) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    - [6/9] Resaving Team United (3156) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    - [7/9] Resaving Team Eternity (3106) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    - [8/9] Resaving Team Kickass (2847) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    - [9/9] Resaving Team Reloaded (2672) ... error: Setting unknown property: craft\elements\Entry::semesterlyPrice
    ```

[i-just]

Hi, thanks for reaching out.

In terms of the values still being there after removing the field from an entry type - this is the expected behaviour at this point: #12069 (comment).

I have created a PR to make the money filter fail silently if anything but the Money type value is provided.

Also, just in case you find it helpful, you can still check for an entry type in an overview template. You can use entry.type.handle to check what entry type you’re on and decide if you should show the money field based on that. I hope this helps!

[michtio]

@i-just a few remarks.

1. Failing the filter silently, I don't think that is a good idea. As people might use the filter wrongly, and no error would be thrown or feedback would be given.

2. The issue you pointed to, mentions php craft resave/entries --section=mySection --type=myType --set=myFieldHandle --to=:empty: to clear out values as a solution. As shown above in the issue, that fails once the field isn't there anymore. if the templates can still read the content and do a fail on the filter for being a string rather than a money object, then the CLI command should work, or the field should still somehow be returned as a Money/Money object and not a string. We are somewhat in a limbo... for resaving entries the field doesn't exist, but for parsing templates it does exist.

3. I don't want to manually code every single entry type when I add one and create logic where there shouldn't be any. I currently have 6 different entry types, with only 3 having that money field / name, others having now other money fields. I don't want to go and write "complicated" if/else statements based on entry types.

In my opinion there is still something fundamentally wrong either way or another. You couldn't expect people to go into the database and clear all values to make it work / have the money filter fail silently on places where it shouldn't be used.

[michtio]

On the above notes @brandonkelly , just an idea:

Would it be possible whenever a field is parsed -- entry type is known, fieldLayouts are linked already to it, is to check if the field is actually still part of the entry type's fieldLayout, if it's not known to the fieldLayout of said entry type anymore, twig should not parse it and throw an error "field is unknown" rather than actually show content.

This could be an in between in the discussion if content should be removed from the database or not. The content could still stay in the database, it's just the value that wouldn't be parsed anymore and treated as "unknown property"

[brandonkelly]

This is resolved for the next Craft 3 and 4 releases, via #12578.

[brandonkelly]

Craft 3.7.64 and 4.3.7 have been released with that fix.

[sebastian-lenz]

I think this fix introduces a new bug: It seems to me that Craft now no longer eager loads matrix blocks.

Due to the filter applied here all matrix blocks are removed from the array $filteredElements as the filter tries to fetch the field stored in $plan->handle here but the handle contains the matrix block type as a prefix (e.g. $plan->handle contains something like "myBlockType:myField" and not "myField"). The call to getFieldByHandle therefore always returns null for matrix block queries.

[boboldehampsink]

Ah just left a comment b687738 but yeah, as @sebastian-lenz says, eager loading is broken.

[i-just]

Hi, thanks for reporting and the details. Much appreciated! A new PR to fix this issue has been created.

[boboldehampsink]

This still isn't fixed for me in 4.3.8...

[brandonkelly]

@boboldehampsink Can you elaborate?

[boboldehampsink]

In my case this:

        // Get products from matrix
        $arProducts = $entry->arProducts->with([
            ['product:product.product.productLine', ['site' => $site]],
        ])->all();

        foreach ($arProducts as $arProduct) {
            $variant = $arProduct->product[0] ?? null;

            if ($variant) {
                $product = $variant->getProduct();
                $productLine = $product->productLine[0] ?? null;
...

Used to work before 4.3.7, and doesn't work on 4.3.8

[brandonkelly]

@boboldehampsink Sorry, that was my fault (bad merge). 4.3.8.1 is out now with a proper fix.

[brandonkelly]

Note that if you updated to 4.3.8 via the control panel, you will be affected by this bug, which was also fixed in 4.3.8.1. If so you will need to run composer update to fix.

[boboldehampsink]

Works now, thanks!

[i-just]

Thanks for confirming! :)

[boboldehampsink]

Since installing 4.3.8 I'm getting lots of other eagerloading related errors, see https://robuust-6t.sentry.io/share/issue/b17a72d652334f53a4ecba27f89d99d2/

[gioppe]

Same here. Getting the same error pretty much everywhere :(

[myleshyson]

4.3.8.1 fixes some eagerloading issues and is causing others for us. We're gonna stay on 4.3.6.1 till this stabilizes. Re-reading this issue and wondering if maybe the money filter should just be a little more flexible? Seems like different solutions that update eagerloading logic are causing lots of unintended consequences.

[myleshyson]

Like maybe if the money filter accepted mixed $money instead of ?Money $money, and then the check being $money instanceof Money?.

Or maybe the element should return null if that field isn't in the field layout anymore, instead of whatever is in the content table. It does seem kinda weird that you can remove a field from an entry type, and still get a value.