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

Add Souin as available provider to the existing system #4540

Closed
wants to merge 1 commit into from
Closed

Add Souin as available provider to the existing system #4540

wants to merge 1 commit into from

Conversation

darkweak
Copy link
Contributor

@darkweak darkweak commented Nov 9, 2021

Q A
Branch? main
Tickets #4512
License MIT
Doc PR api-platform/docs#...

Add Souin as an available purger provider instead of the Varnish one to be able to use natively Souin as a cache system inside the Caddy reverse-proxy and refactor the AddTagsListener to be able to customise the HTTP header tag and the separator instead of the forced Cache-Tag from Cloudflare.

@soyuka
Copy link
Member

soyuka commented Nov 9, 2021

note that the ci is broken we're trying to fix underlying issues

@darkweak
Copy link
Contributor Author

@soyuka @dunglas this PR is ready for review

features/http_cache/tags.feature Outdated Show resolved Hide resolved
src/Core/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
src/Core/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
src/Core/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
tests/Core/HttpCache/EventListener/AddTagsListenerTest.php Outdated Show resolved Hide resolved
@darkweak
Copy link
Contributor Author

The tests on the main branch break the PHPUnit ones on PRs. Due to the bb2180f commit.

@darkweak darkweak requested a review from soyuka November 12, 2021 11:01
@darkweak
Copy link
Contributor Author

ping @dunglas to review as we discussed with @soyuka

src/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
tests/HttpCache/EventListener/AddTagsListenerTest.php Outdated Show resolved Hide resolved
tests/HttpCache/SouinPurgerTest.php Outdated Show resolved Hide resolved
@darkweak
Copy link
Contributor Author

@soyuka I reverted the Cache-Tag into Cache-Tags. Can you re-review for the last time?
Ty

@darkweak darkweak requested a review from soyuka December 16, 2021 12:39
src/HttpCache/EventListener/AddTagsListener.php Outdated Show resolved Hide resolved
src/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
src/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
src/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
src/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
src/HttpCache/SouinPurger.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
@darkweak darkweak requested a review from dunglas January 4, 2022 15:53
@darkweak
Copy link
Contributor Author

darkweak commented Mar 3, 2022

Hi there, what about this?

@BeyerJC
Copy link
Contributor

BeyerJC commented May 10, 2022

Hey, is there a Chance this feature will be in 2.7 ?

@darkweak
Copy link
Contributor Author

I don't think because they never answered 🙃

@darkweak
Copy link
Contributor Author

darkweak commented Jul 4, 2022

@soyuka @dunglas Can I have a review please ? Everything is working as expected with tests. It would be a great add-on on the v3 to deal with the caddy native HTTP cache system.

@dunglas
Copy link
Member

dunglas commented Jul 5, 2022

We're focus on releasing API Platform 3 (which is in beta). We'll review this PR as well as other new features when API Platform 3 will be tagged.

@darkweak
Copy link
Contributor Author

darkweak commented Sep 22, 2022

Ready to review/merge.

@darkweak darkweak requested review from maxhelias, dunglas and soyuka and removed request for dunglas, soyuka and maxhelias September 22, 2022 10:21
@darkweak darkweak requested review from soyuka and removed request for dunglas September 22, 2022 13:15
@darkweak
Copy link
Contributor Author

ping @dunglas @soyuka

@darkweak
Copy link
Contributor Author

Can I have a last review please?

$definitions[] = $definition;
}
$serviceName = $config['http_cache']['invalidation']['purger'];
switch ($serviceName) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we prefer match now

Copy link
Contributor Author

@darkweak darkweak Nov 25, 2022

Choose a reason for hiding this comment

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

The switch case allows us to use varnish by default.

src/Symfony/Bundle/Resources/config/http_cache_purger.xml Outdated Show resolved Hide resolved
tests/Metadata/Extractor/YamlExtractorTest.php Outdated Show resolved Hide resolved
@soyuka
Copy link
Member

soyuka commented Nov 5, 2022

Nice! A few comments lefts but we're almost good to merge imo! Thanks @darkweak sorry for the delays!

@dkarlovi
Copy link
Contributor

@soyuka note this discussion: darkweak/souin#277 (comment)

It seems Souin will start passing any request as soon as the Authorization header (or Cookie) is present in the request, it doesn't provide (nor plans to provide) any sort of "user context"-like feature FOS HTTP Cache bundle, which IIRC APIP relied on before.

@darkweak
Copy link
Contributor Author

Ready for the merge @soyuka

@divine
Copy link
Contributor

divine commented Dec 16, 2022

@soyuka note this discussion: darkweak/souin#277 (comment)

It seems Souin will start passing any request as soon as the Authorization header (or Cookie) is present in the request, it doesn't provide (nor plans to provide) any sort of "user context"-like feature FOS HTTP Cache bundle, which IIRC APIP relied on before.

Well, then there is no reason that souin should be integrated into APP.

offtopic:

@darkweak would like to consider changing the souin logic?

Thanks!

@darkweak
Copy link
Contributor Author

darkweak commented Dec 16, 2022

@divine note this PR darkweak/souin#283

Anyway, TBH the api platform team won't merge this PR I guess.

@soyuka
Copy link
Member

soyuka commented Dec 17, 2022

merci @darkweak

@soyuka soyuka closed this Dec 17, 2022
@soyuka
Copy link
Member

soyuka commented Dec 17, 2022

I cherry-picked it in my PR just to be a bit less provider specific. souin will have native support from API Platform 3.1 (coming out mid-January) you can try it using dev-main for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

7 participants