Comments

jan.stoeckler created an issue. See original summary.

jan.stoeckler’s picture

StatusFileSize
new6.87 KB

Let's see if this does the job.

jan.stoeckler’s picture

StatusFileSize
new424 bytes

Missed some files for the patch.

jan.stoeckler’s picture

jan.stoeckler’s picture

StatusFileSize
new15.53 KB

Sorry for the confusion. This should work.

jan.stoeckler’s picture

Status: Needs work » Needs review
Issue tags: -drupalcon +dublin2016 drupalcon

Patch seems to be working nicely, setting this to needs review.

marvin_b8’s picture

Status: Needs review » Reviewed & tested by the community
gábor hojtsy’s picture

Issue tags: -dublin2016 drupalcon +Dublin2016
vijaycs85’s picture

Thanks for working on this issue @jan.stoeckler. Here is my review.

  1. +++ b/fastly_add_soft_purge.patch
    @@ -0,0 +1,168 @@
    + ¶
    ...
    + ¶
    

    minor: Remove space

  2. +++ b/src/EventSubscriber/AddSoftPurgeHeaders.php
    @@ -0,0 +1,85 @@
    +    $cache_control_header .= ', stale-while-revalidate=' . $config->get('stale_while_revalidate_value');
    ...
    +      $cache_control_header .= ', stale-if-error=' . $config->get('stale_if_error_value');
    

    IMHO, we should Surrogate-Control header instead of Cache-Control. This way, we don't change the default behaviour. Ref: https://docs.fastly.com/guides/tutorials/cache-control-tutorial#when-cac...

jan.stoeckler’s picture

Thanks @vijaycs85 for your review, I'll get right on that.

vijaycs85’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @jan.stoeckler. updating status accordingly.

jan.stoeckler’s picture

StatusFileSize
new9.54 KB
new1.11 KB

Sorry for taking so long, here it is.

jan.stoeckler’s picture

StatusFileSize
new9.44 KB
new1.51 KB

And a small correction.

jan.stoeckler’s picture

Status: Needs work » Needs review

Status update.

jan.stoeckler’s picture

StatusFileSize
new9.78 KB
new1.85 KB

And another correction.

jan.stoeckler’s picture

StatusFileSize
new9.54 KB
new1.54 KB

Fix another misguided attempt.

vijaycs85’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the updates @jan.stoeckler. It looks good.

vijaycs85’s picture

Can we get this in please? It's been in RTBC for a while.

leon kessler’s picture

Sorry for the delay....
I believe the Fastly team were going to be maintaining this module. I was the maintainer of the D7 version, but can help out with the D8 version too.

Patch looks good, just one thing that looks odd....

+++ b/src/Form/FastlySettingsForm.php
@@ -88,6 +94,60 @@ class FastlySettingsForm extends ConfigFormBase {
+//      '#title' => $this->t('Stale if error'),

Should this be removed or uncommented?

vijaycs85’s picture

@Leon Kessler, sure, would you mind apply patch and remove the commented line? We can issue another patch without that line, but it's not that great chance. can go as part of commit.

leon kessler’s picture

Status: Reviewed & tested by the community » Fixed

Okay pushed to dev.

Tried to create a 8.x-3.1 release as well, but I do not have sufficient privileges.

vijaycs85’s picture

awesome! thank you @Leon Kessler.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.