Only default locale fetch for specific database fields

satrun77 commented 8 months ago

Module version(s) affected

7.0.0

Description

All localised pages display some of their content (database fields) from SiteTree_Live instead of SiteTree_Localised_Live

This only happens to some fields with a specific configuration setup. The issue is not limited to this version.

How to reproduce

Fresh installation of the CMS with fluent.
Setup basic fluent
Create more than one-page type with any time of db fields

Create an extension with a DB field

class PageExtension extends DataExtension {
private static array $db = [
    'SubTitle' => 'Varchar',
];

// ....
}

Apply the extension to each of the page types. This configuration causes the issue.
```
PageType2:
extensions:
- PageExtension
```

PageType3: extensions:

PageExtension

- Localisation does not work for `SubTitle` only. The CMS interface shows the value stored in `PageType2_Live` for all localisations. The data is stored in the DB as expected. 
- The issue is that the DB query does not fetch the data from the correct table.
- The issue is in code https://github.com/tractorcow-farm/silverstripe-fluent/blob/4/src/Extension/FluentExtension.php#L998
- The SQL fragment is something like this
```sql
CASE
    WHEN "SiteTree"."ClassName" IN ('Page', '...') THEN "PageType2"."SubTitle"
    WHEN "SiteTree"."ClassName" IN ('...') THEN "PageType3"."SubTitle"
    ELSE NULL
END AS SubTitle

The code is unable to find "PageType2"."SubTitle" and "PageType3"."SubTitle"

Note: this issue does not exists, if the extension is applied to Page or SiteTree

Possible Solution

The following is a quick working fix

protected function detectLocalisedTableField($tables, $sql)
{
        $pattern = '/THEN\s+("(?<table>[\w\\\\]+)"\."(?<field>\w+)")?\s+/miU';

        if (str_starts_with($sql, 'CASE WHEN')
            && preg_match_all(
                $pattern,
                $sql,
                $matches,
                PREG_SET_ORDER
            )
            && isset($matches[0])
        ) {
            foreach ($matches as $match) {
                $table = $match['table'];
                $field = $match['field'];

                // Ensure both table and this field are valid
                if (!$tables[$table] || !in_array($field, $tables[$table], true)) {
                    return [null, null, false];
                }

                return [$table, $field, true];
            }
        }

        return parent::detectLocalisedTableField($tables, $sql);
 }

I will try to make a PR when I can.

Additional Context

No response

Validations

[X] Check that there isn't already an issue that reports the same bug
[X] Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

tractorcow commented 7 months ago

Oh right, this is the "eager" loading behaviour of silverstripe when you are selecting child fields from a base class. It's an incredibly fragile piece of code as it is, so I'm familiar with it. Specifically it's collidingFields.

@satrun77 can you show me the query you're using in code (i.e. your SiteTree::get()->filter etc...) that causes the above error. Normally SiteTree::get() should only query fields on the base class, and not any of the nested class fields, so I'm wondering what code is attempting to preload all the child fields. Is that behaviour changed in SS5?

One temp fix here is to rely on lazy loading, and not try to query both fields from each table up front.

Another fix could be to simply add your common field to the base class, not the adjacent siblings, so you don't run into the issue with fields colliding at all.

satrun77 commented 7 months ago

The issue I had was from loading the page type. Not using a custom query. I have a few page types with an extension that includes a few fields for the hero area.

Without the above code change. The hero area for all localised pages is the same value defined in Page_Live table.

tractorcow commented 7 months ago

Hm, that sounds like a pain. Best advice I can give for the mean time is to move the extension to Page for now, until we can investigate and come up with a proper fix.

tractorcow-farm / silverstripe-fluent