Skip to content

Conversation

@michalsn
Copy link
Member

Description
This PR introduces a new configuration property, Config\Cache::$cacheStatusCodes, which allows developers to explicitly control which HTTP response status codes are cached.

This is important because not all responses should be cached by default. In practice, applications may want to cache responses beyond 200 OK (for example, 404), or the opposite - prevent caching of certain status codes to avoid serving stale or misleading responses. Making this configurable gives developers good control over cache behavior.

Minor BC break:
The PageCache filter constructor now accepts an optional Cache parameter to allow dependency injection (primarily for testing). This should not affect most users, as the parameter has a default value and existing usage continues to work unchanged.

Checklist:

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

@michalsn michalsn added enhancement PRs that improve existing functionalities breaking change Pull requests that may break existing functionalities 4.7 labels Dec 27, 2025
Copy link
Contributor

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

Just a quick note: an empty array [] currently means cache all status codes, but it might read as cache nothing at first glance. Using something like [Cache::ALL_STATUS_CODES] or ['*'] could make it clearer.

@michalsn
Copy link
Member Author

I considered it, but storing all status codes in one place seems counterproductive. Using ['*'] would also require a mixed property type, which could lead to confusion when users start declaring status codes as strings rather than integers. Since the behavior of an empty array is documented both in the config file and the user guide, this feels like the correct approach.

@datamweb
Copy link
Contributor

but storing all status codes in one place seems counterproductive

I wasn’t suggesting enumerating all status codes.
My intent was a single sentinel value (e.g. a constant or numeric marker) that clearly communicates all statuses, while remaining type-safe.

public const ALL_STATUS_CODES = -1;

That said, I understand the preference to rely on documentation.

@michalsn
Copy link
Member Author

Oh, okay - thanks for the clarification. If more users agree that having something like ALL_STATUS_CODES is preferable, then I'm happy to make the change.

@michalsn michalsn force-pushed the feat/cache-status-codes branch from 52298aa to 97863a1 Compare December 28, 2025 08:49
Copy link
Contributor

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

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

I don't see the point in adding a constant. The empty array looks correct. In the future, we can update if receive a bug report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.7 breaking change Pull requests that may break existing functionalities enhancement PRs that improve existing functionalities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants