Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only default locale fetch for specific database fields #817

Open
2 tasks done
satrun77 opened this issue Jan 10, 2024 · 3 comments
Open
2 tasks done

Only default locale fetch for specific database fields #817

satrun77 opened this issue Jan 10, 2024 · 3 comments

Comments

@satrun77
Copy link
Contributor

satrun77 commented Jan 10, 2024

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
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

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

tractorcow commented Jan 11, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants