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

Support static caching in Statamic #1440

Merged
merged 7 commits into from
Sep 25, 2023
Merged

Conversation

jasonvarga
Copy link
Contributor

Closes #1418

This PR adds a new driver for Statamic v3+.

Starting in v3, Statamic is just a Laravel package so we didn't have a need for a dedicated driver - the Laravel driver works fine. However as pointed out in #1418, our "static caching" feature wouldn't function 100% as expected locally because it assumes some custom server config. This driver "fakes" it.

In addition to the driver itself, it needs to be loaded before the Laravel driver. Otherwise, the Laravel driver will be used since a Statamic site also qualifies as a Laravel site.

Lastly, @aerni mentioned in the issue that we should call this driver "Statamic" and the previous one "StatamicV2" - I agree but it would be a breaking change. Not a big deal.

@drbyte
Copy link
Contributor

drbyte commented Aug 28, 2023

Lastly, @aerni mentioned in the issue that we should call this driver "Statamic" and the previous one "StatamicV2" - I agree but it would be a breaking change. Not a big deal.

I'd be okay with (and prefer) that change (rename current one to V2, and let this V3 be the unversioned name) ... since I can't think of a logical case where anyone would have that filename hardcoded anywhere.

@jasonvarga
Copy link
Contributor Author

Can do.

@jasonvarga
Copy link
Contributor Author

Didn't realize there were tests for that - nice. Fixing those.

@jasonvarga
Copy link
Contributor Author

Okay, done now. I've added tests and adjusted them so they all have appropriate fixtures corresponding to the renamed drivers.

@drbyte
Copy link
Contributor

drbyte commented Aug 28, 2023

FWIW, in my testing I get the same results with either of these strategies:

The one currently in this PR:

+        $drivers[] = 'Specific\StatamicValetDriver';
         $drivers[] = 'LaravelValetDriver';
         $drivers = array_unique(array_merge($drivers, $specificDrivers));

and the other idea suggested in the related issue:

-        $drivers[] = 'Specific\StatamicValetDriver';

-        $drivers[] = 'LaravelValetDriver';
         $drivers = array_unique(array_merge($drivers, $specificDrivers));
+        $drivers[] = 'LaravelValetDriver';

Each approach has its pros and cons:

  • hardcoding is more rigid, but is more certain (and I'm okay with that, for same reasons Matt already mentioned)
  • pushing the Laravel one down after the array_merge "might" have unexpected side-effects, although I'm not finding any in the testing of a few drivers that I've simulated.

In the end, I don't mind which approach we use here. I'm fine with the hardcoding of the Statamic driver, as committed already.
Just pointing out the additional testing results.

@jasonvarga
Copy link
Contributor Author

Thanks. I'll write a test somewhere for the first/current change. I'd rather not move the Laravel driver in case it changes something.

@drbyte
Copy link
Contributor

drbyte commented Aug 28, 2023

Totally understood.

Will leave it to @mattstauffer to merge etc.

@mattstauffer mattstauffer merged commit 2288348 into laravel:master Sep 25, 2023
@mattstauffer
Copy link
Collaborator

Thanks @jasonvarga and @drbyte!

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.

Static files don't load in Statamic 3/4+
3 participants