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

FIX Preview link when no locale set #861

Merged

Conversation

emteknetnz
Copy link
Contributor

Issue silverstripe/.github#272

Fixes vendor/silverstripe/cms/tests/behat/features/preview-a-page.feature:12 - which breaks when silverstripe/recipe-kitchen-sink is run including silverstripe/cms behat tests.

The issue that behat is running into is that the preview link will not show anything as no locale has been defined in a behat context

Copy link
Contributor

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to feel really flaky.

I've done a root cause analysis and this PR is what causes the problem that results in needing this "temp locale" in the first place.

This fixes it without needing a temp locale:

     public function updatePreviewLink(&$link): void
     {
         $owner = $this->owner;
-        $info = $owner->LocaleInformation(FluentState::singleton()->getLocale());
+        $locale = FluentState::singleton()->getLocale();
+        if ($locale === null || $locale === '') {
+            return;
+        }
+        $info = $owner->LocaleInformation($locale);

         if (!$info->getSourceLocale()) {
             $link = null;
         }
     }

With this change, if there is no current locale (i.e. locales haven't been set up yet) it won't update the preview URL. It'll only update it when there is a current locale (i.e. locales have been set up).

@emteknetnz
Copy link
Contributor Author

Updated

Copy link
Contributor

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also revert the changes that were made in 2714bff and b14687a - the temporary locale isn't needed anymore.

@emteknetnz
Copy link
Contributor Author

Surprisingly yes, looks like the previous CI failure was actually just from rendering the preview link from a template.

Reverted as second commit, so do not squash merge

Copy link
Contributor

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GuySartorelli GuySartorelli merged commit 80293d2 into tractorcow-farm:7 Jun 11, 2024
10 checks passed
@GuySartorelli GuySartorelli deleted the pulls/7/fix-preview branch June 11, 2024 00:09
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

Successfully merging this pull request may close these issues.

2 participants