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

refactor: extract ResponseCache class for Web Page Caching #7644

Merged
merged 13 commits into from
Jul 5, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jun 30, 2023

Description

  • extract ResponseCache class
    • to make it easier to extend the page caching
  • fix incorrect type definitons for request

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added refactor Pull requests that refactor code 4.4 labels Jun 30, 2023
@iRedds
Copy link
Collaborator

iRedds commented Jun 30, 2023

The service returns the same object. Does it make sense to create an additional property?

@kenjis kenjis marked this pull request as draft June 30, 2023 09:50
@kenjis kenjis added breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them labels Jun 30, 2023
@kenjis kenjis force-pushed the refactor-PageCache branch 2 times, most recently from 911db4e to dba3d1a Compare June 30, 2023 11:23
@iRedds
Copy link
Collaborator

iRedds commented Jun 30, 2023

I'd like to suggest a change.

  1. Renaming a class and methods.
  • rename class PageCache to ResponseCache
  • rename method cachePage to make
  • rename method getCachedResponse to get
  1. Move the CodeIgniter::$cacheTTL property to the new page caching class. That is, all functionality will be processed by one class.
    The Controller::cachePage() method could then look like this.
protected function cachePage(int $time)
{
    Services::pagecache()->ttl($time);
}
  1. I don't know if it makes sense for the generateCacheKey method to be public.

@kenjis
Copy link
Member Author

kenjis commented Jul 1, 2023

Thank you for your comment.

What about the namespace? Should be in CodeIgniter\HTTP?

@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Jul 1, 2023
@iRedds
Copy link
Collaborator

iRedds commented Jul 1, 2023

What about the namespace? Should be in CodeIgniter\HTTP?

I think the namespace should stay the same.

@kenjis kenjis changed the title refactor: extract PageCache class for Web Page Caching refactor: extract ResponseCache class for Web Page Caching Jul 3, 2023
Comment on lines 530 to 532
if ($this->pageCache->getTtl() > 0) {
$this->pageCache->make($this->request, $this->response);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for considering my suggestion, but in this snippet, I meant a slightly different approach.

Suggested change
if ($this->pageCache->getTtl() > 0) {
$this->pageCache->make($this->request, $this->response);
}
$this->pageCache->make($this->request, $this->response);

and then the decision on the need to cache the Response is made by the caching class.

    public function make($request, ResponseInterface $response): bool
    {
        if ($this->ttl === 0) {
            return ; // bool (false or true)
        }

This is how I see the implementation, but I don't know if my approach is correct or 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.

If $ttl is 0, there is no need to cache it, so that seems a little better.

Done.

@iRedds
Copy link
Collaborator

iRedds commented Jul 3, 2023

I have a little doubt, but it seems to me that there is no point in caching the response for the CLI.

It seems to me that the current documentation is sufficient, since in fact nothing has changed for the end developer.

Fixes the following errors:
 ------ ------------------------------------------------------------------
  Line   system/CodeIgniter.php
 ------ ------------------------------------------------------------------
  529    Parameter #1 $request of method
         CodeIgniter\Cache\PageCache::cachePage() expects
         CodeIgniter\HTTP\CLIRequest|CodeIgniter\HTTP\IncomingRequest,
         CodeIgniter\HTTP\Request|null given.
  685    Parameter #1 $request of method
         CodeIgniter\Cache\PageCache::getCachedResponse() expects
         CodeIgniter\HTTP\CLIRequest|CodeIgniter\HTTP\IncomingRequest,
         CodeIgniter\HTTP\Request|null given.
 ------ ------------------------------------------------------------------
@kenjis
Copy link
Member Author

kenjis commented Jul 3, 2023

Added changelog.

@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Jul 3, 2023
@kenjis
Copy link
Member Author

kenjis commented Jul 3, 2023

I have a little doubt, but it seems to me that there is no point in caching the response for the CLI.

Indeed, I am not sure if there is a use case.
However, it is a feature that currently exists, so it cannot be removed all of the sudden.

@kenjis kenjis marked this pull request as ready for review July 3, 2023 05:24
tests/system/CodeIgniterTest.php Show resolved Hide resolved
user_guide_src/source/changelogs/v4.4.0.rst Outdated Show resolved Hide resolved
system/Cache/ResponseCache.php Outdated Show resolved Hide resolved
Co-authored-by: MGatner <mgatner@icloud.com>
@kenjis kenjis requested a review from paulbalandan July 4, 2023 22:41
@kenjis kenjis merged commit 0d15487 into codeigniter4:4.4 Jul 5, 2023
48 of 51 checks passed
@kenjis kenjis deleted the refactor-PageCache branch July 5, 2023 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants