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 Ensure everything gets flushed when flushing from sake #11436

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

GuySartorelli
Copy link
Member

Ensures Flushable classes get flushed when flush booting the kernel, regardless of where that's happening from.

Issue

Comment on lines 20 to 17
if ((method_exists($kernel, 'isFlushed') && $kernel->isFlushed())) {
if ($kernel->isFlushed()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

That method is part of the Kernel interface, so no need for the old method_exists check.

@@ -113,4 +114,16 @@ public function testReplicaDatabaseVarsLoaded()
'password' => 'hi_people',
], $configs['replica_01']);
}

public function testImplementorsAreCalled()
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the exact test (with some code quality improvements) that used to live in FlushMiddlewareTest

@GuySartorelli
Copy link
Member Author

CI failures are identical to what's happening on the 6 branch - see https://github.com/silverstripe/silverstripe-framework/actions/runs/11448877901/job/31853334275

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Code is fine, though FlushMiddleware just seems pointless now, just delete it and put the following in HTTPCacheControlMiddleware::process()

$kernel = Injector::inst()->get(Kernel::class);
if ($kernel->isFlushed()) {
  $this->disableCache(true);
}

And deprecate in CMS 5.4.0, changelog entry, remove from yml, etc

Note there's also some docs that need to updated regardless, e.g. 01_Flushable.md is incorrect regardless if we remove FlushMiddleware or not, so do a search in the codebase for "FlushMiddleware"

Also I don't want to merge anything else into CMS 6 until we've fixed up all the broken builds, once that's done could you please re-run tests to confirm that things are green

@GuySartorelli
Copy link
Member Author

once that's done could you please re-run tests to confirm that things are green

I assume you mean in this PR specifically lol

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Oct 22, 2024

just delete it ... and deprecate in CMS 5.4.0, changelog entry, remove from yml, etc

I was avoiding unnecessary breaking changes, because this was meant to be just a quick bugfix. I'll put this back in ready, I've got way too many things running concurrently right now to do a full breaking change set of PRs for this.

@GuySartorelli
Copy link
Member Author

Updated, CMS 5 PRs incoming

@emteknetnz emteknetnz merged commit c1a51aa into silverstripe:6 Oct 25, 2024
10 checks passed
@emteknetnz emteknetnz deleted the pulls/6/flush-on-sake branch October 25, 2024 01:28
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