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

[RTM] Enable the FOS cache bundle #67

Merged
merged 4 commits into from
Jun 9, 2018
Merged

[RTM] Enable the FOS cache bundle #67

merged 4 commits into from
Jun 9, 2018

Conversation

Toflar
Copy link
Member

@Toflar Toflar commented Apr 13, 2018

While enabling the FOSHttpCacheBundle I noticed that configuration for purging the tags is still a bit cumbersome so I worked on this (FriendsOfSymfony/FOSHttpCache#419) and I am trying to get this into the bundle because it will greatly simplify our integration.
I'll keep this PR up-to-date with the changes on FOSHttpCache(Bundle). Once - hopefully - this gets all released as version 2.3, I'll finish this PR so it can be merged.

@Toflar Toflar changed the title [WIP] Enable the FOS cache bundle [RTM] Enable the FOS cache bundle Apr 25, 2018
@Toflar
Copy link
Member Author

Toflar commented Apr 25, 2018

FOSCacheBundle 2.3 has been released, this PR is ready to be merged.

@Toflar Toflar added this to the 4.6.0 milestone Apr 26, 2018
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

Instead of calling setHttpCache in the entry point, I think the ContaoCache class should make that call in it's own constructor (if the passed-in kernel implements the interface).

@Toflar
Copy link
Member Author

Toflar commented Apr 26, 2018

This is not possible because the interface only has a getHttpCache(), it has no setHttpCache() because it doesn't care.

{
$kernel = $this->mockKernel($this->getTempDir());

$this->assertInstanceOf(HttpCacheProvider::class, $kernel);
Copy link
Member

Choose a reason for hiding this comment

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

This test should go into the testInstantiation() method.

*/
public function __construct(KernelInterface $kernel, string $cacheDir = null)
public function __construct(ContaoKernel $kernel, string $cacheDir = null)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this. Why don't we use HttpCacheAware instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no HttpCacheAware?

Copy link
Member

Choose a reason for hiding this comment

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

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's a trait, you can't typehint a trait?

Copy link
Member

Choose a reason for hiding this comment

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

Sure?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, which is exactly what we want. You shouldn't be able to use a different kernel other than ContaoKernel for ContaoCache. We cannot typehint for an interface either :)

Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't be able to use a different kernel other than ContaoKernel

Why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's our cache with our kernel? If you want to do something else, you're very likely not only adjusting the kernel but also the cache. Also, keep in mind that this is a managed edition.

Copy link
Member

Choose a reason for hiding this comment

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

Both kernel and cache are set in the entry point and can therefore be adjusted even in the managed edition.

@contao/developers What's your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

It's a Contao Managed Edition, you're not supposed to change anything. If you really need to, your kernel must extend ContaoKernel, that's totally fine for me. If you really don't want that, then simply also create your own cache kernel.

@leofeyer leofeyer merged commit 2394009 into contao:master Jun 9, 2018
@leofeyer
Copy link
Member

leofeyer commented Jun 9, 2018

Thank you @Toflar.

@Toflar Toflar deleted the enable-fos-cache-bundle branch June 9, 2018 11:19
leofeyer pushed a commit to contao/core-bundle that referenced this pull request Jun 9, 2018
Description
-----------

Automatically invalidates DCA entries when users edit them in the back end.
For testing purposes I've added corresponding tags on the page response.

Depends on contao/manager-bundle#67 which itself depends on other PR's again. Leaving it here for reference and to be updated.

Commits
-------

539bb9f Introduced auto cache tag invalidation on DCAs for better DX
c877b8b Merge branch 'master' into auto-dca-cache-tag-invalidation
26fbbb3 Renamed the tags
3804d9d Fix the coding style.
christian-kolb pushed a commit to christian-kolb/contao that referenced this pull request Oct 11, 2018
contao#1478).

Description
-----------

Automatically invalidates DCA entries when users edit them in the back end.
For testing purposes I've added corresponding tags on the page response.

Depends on contao/manager-bundle#67 which itself depends on other PR's again. Leaving it here for reference and to be updated.

Commits
-------

539bb9fe Introduced auto cache tag invalidation on DCAs for better DX
c877b8b0 Merge branch 'master' into auto-dca-cache-tag-invalidation
26fbbb3a Renamed the tags
3804d9df Fix the coding style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants