Broken builds and merge-ups

[github-actions]

This is an automatically created issue used to remind the Silverstripe CMS Squad to look at broken builds and merge-ups every Tuesday.

It was created by the .github/workflows/rhino-issue.yml workflow in the silverstripe/.github repository.

Triage instructions

1. Put on the following label:

• type/other

2. Move this issue to the "Ready" column on our internal zenhub board

Broken builds:

• https://rhino.silverstripe.org/?t=builds

Merge-ups:

• https://rhino.silverstripe.org/?t=merge-ups

Steve PRs

• FIX Run ANALYZE TABLE before fetching table stats silverstripe-framework#11334

PRs

• FIX Correctly clear table and reset auto increment silverstripe-framework#11333
• MNT Ignore lint failure we can't fix yet silverstripe-mfa#558
• MNT Ignore lint failure we can't fix cwp-agencyextensions#131
• MNT Ignore pgpstan errors we can't fix silverstripe-admin#1808
• MNT Ignore phpstan errors we can't fix silverstripe-assets#627
• MNT resolve translation lint failures silverstripe-blog#776
• MNT Ignore phpstan error we can't fix silverstripe-userforms#1316
• MNT Ignore phpstan error we can't fix symbiote/silverstripe-queuedjobs#442
• MNT Ignore phpstan error we can't fix symbiote/silverstripe-gridfieldextensions#413
• MNT Run module-standardiser gha-add-pr-to-project#12
• FIX Correct trailing slash for locale in another domain tractorcow-farm/silverstripe-fluent#872
• FIX Correct trailing slash for subsite in another domain silverstripe-subsites#590

Important

After merging the PRs, assign to Guy to do the following:

• Raise a CMS 6 PR to update __CLASS__ to __TRAIT__ as per MNT Ignore lint failure we can't fix yet silverstripe-mfa#558
• Confirm builds are going green

[GuySartorelli]

Not done yet, reopening

[GuySartorelli]

@emteknetnz Since you merged tractorcow-farm/silverstripe-fluent#872 and silverstripe/silverstripe-subsites#590 without responding to the content in the PR descriptions I'll bring it up here:

The fix in those PRs was very narrow in scope, and likely doesn't cover a bunch of scenarios - especially for fluent where the tests only covered SiteTree and not other data objects.

A more robust solution would be to implement the updateIsSiteUrl() extension hook for Director for both modules in a minor release. This would allow us to say "yes, any domain we know about for any locale or subsite in this project is a 'site URL'".
There may be flow-on effects for that though - it really depends if by "site URL" it means "this project" or if it means "this locale/this subsite".

Right now if it's the latter, it'll be wrong whenever domains aren't used to separate locales and subsites, so the result would be inconsistent. However, if we change it to always return true if the URL is for any subsite or locale, this has potential to cause problems if someone is using that method to distinguish between them in a project which does use domains.

Personally I'm in favour of making the change (in a minor release), but I want at least one other opinion before I do that.

FYI @tractorcow @silverstripe/core-team

[kinglozzer]

I rarely use fluent, and I think I’ve only used subsites once or twice ever, so I don’t have much insight to offer here. I think the question I’d ask is whether the issue(s) that highlighted this have been fixed/worked around? If yes, and there’s a non-zero risk that we could break things for people, I’d be inclined to delay the change until a major release

[GuySartorelli]

I think the question I’d ask is whether the issue(s) that highlighted this have been fixed/worked around?

Yes, the very specific CI failures have been resolved. But as I mentioned above those CI failures and the corresponding fixes were very narrow in scope.

If for example you have a fluent locale with domain www.example.com and your main site is www.example.co.nz, any case where you put the fluent locale through Controller::normaliseTrailingSlash() which doesn't come from one of the two code paths that got patched will still have the incorrect trailing slash, if the domain for that locale doesn't match whatever director things is the current correct domain for the site.
This means and non-SiteTree data object links, controller actions, hardcoded URIs, you name it.

[emteknetnz]

A more robust solution would be to implement the updateIsSiteUrl() extension hook for Director for both modules in a minor release. This would allow us to say "yes, any domain we know about for any locale or subsite in this project is a 'site URL'".

Seems reasonable, though should be done in a separate card from broken builds