Fix broken caching system when array of allowed parameters used

[JavaDeveloperKiev]

Cache Include Query String in the /Config/Cache.php (line 73) suggest to the user 3 options to choose from:

false = Disabled

true = Enabled, take all query parameters into account. Please be aware that this may result in numerous cache files generated for the same page over and over again.

array('q') = Enabled, but only take into account the specified list of query parameters.

However, last option with array of important parameters just doesn't work. We were using third option, because we had one important parameter (color) on the site, and content were dynamic based on this parameter.

But then we`ve seen huge amount of cache files on our server, thousands and thousands. Turned out, even when using array of parameters in Config/Cache $cacheQueryString option - CodeIgniter was creating new cache file for any requests just like it was set to 'true'. Cache fles generated for the same page over and over again, performance were slow.

Since Google Ads, Bing, Yahoo and all marketing platforms always adding dynamic parameters like 'click_id' for example, 'utm_source', 'utm_medium' and other, cache file was creating for all requests from the paid traffic. It was almost impossible to catch, but we saw weird amount of caching files, and that helped us to see the problem.

I`ve made testing on the clean CodeIgniter 4 installations, latest version. Happened there too.

So I`ve checked the source code, and I saw there there is no difference between true/array options, they just work same. I believe this is very missleading, because Config/Cache showing 'array' option and suggest to use it.

This fixes should help, and take into the account array option.

Thank you

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]

CodeIgniter4/app/Config/Cache.php

Lines 55 to 71 in 9546098

	* --------------------------------------------------------------------------
	* Cache Include Query String
	* --------------------------------------------------------------------------
	*
	* Whether to take the URL query string into consideration when generating
	* output cache files. Valid options are:
	*
	* false = Disabled
	* true = Enabled, take all query parameters into account.
	* Please be aware that this may result in numerous cache
	* files generated for the same page over and over again.
	* array('q') = Enabled, but only take into account the specified list
	* of query parameters.
	*
	* @var bool|string[]
	*/
	public $cacheQueryString = false;

[kenjis]

Thank you for sending a PR!

1. Please fix coding style. See GitHub Action checks.

• https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#php-style
• https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#static-analysis-on-php-code

2. Please add test code.

• https://github.com/codeigniter4/CodeIgniter4/tree/develop/tests#resources

we expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works. If pull requests are not accompanied by relevant tests, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work on the framework than you do. Please make it as painless for your contributions to be included as possible. If you need help with getting tests running on your local machines, ask for help on the forums. We would be happy to help out.
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#contributions

[JavaDeveloperKiev]

kenjis

Sure thing :)

I am going to write a test, but method that generating cache name (and having the bug) is protected.

What is the best practice in CodeIgniter 4, to create a test for protected methods? Should I change it to public to run a test, or do something else?

Thanks!

[kenjis]

Sorry, it is difficult to write test code for it.

What is the best practice in CodeIgniter 4, to create a test for protected methods?

It is fine in this case,

Or integration test like this:

CodeIgniter4/tests/system/CodeIgniterTest.php

Line 569 in 9546098

public function testPageCacheSendSecureHeaders()

Should I change it to public to run a test

No. We cannot change public APIs for bug fixes (as much as possible).

[JavaDeveloperKiev]

Sorry, it is difficult to write test code for it.

What is the best practice in CodeIgniter 4, to create a test for protected methods?

It is fine in this case,

Or integration test like this:

CodeIgniter4/tests/system/CodeIgniterTest.php

Line 569 in 9546098

public function testPageCacheSendSecureHeaders()

Should I change it to public to run a test

No. We cannot change public APIs for bug fixes (as much as possible).

Okay, I`ve created three new tests, for all possible values of the $cacheQueryString in Config/Cache.php

Also the bug is fixed in CodeIgniter.php, and now when $cacheQueryString is set to array of important parameters - it works how it should.

If you run tests in the current dev/master branch, you`ll see that testPageCacheWithCacheQueryStringArray() test is failling, because CodeIgniter creating 5 cache files instead of 3 expected.

Thats pretty critical bug, because its generating thousands of cache files in the production server, and showing not cached version of the pages for all users with unique not important parameters like utm_source/click_id/etc

In my branch its fixed

[kenjis]

Can you rebase?

[JavaDeveloperKiev]

Can you rebase?

Sure, rebased branch, now all commits should be verified

[kenjis]

Oh, now this PR has 108 commits!
What command did you run?

How to rebase:
https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

[JavaDeveloperKiev]

Oh, now this PR has 108 commits! What command did you run?

How to rebase: https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

Not sure what went wrong, I copied commands from instructions, see the snapshot. As I understand, its merged all existing commits from upstream/develop to this branch in last 10 days, and then added mine commits over it.

Want to me create new branch and open another PR instead of this one?

[kenjis]

Want to me create new branch and open another PR instead of this one?

Yes.
I cannot merge this PR because there are commits that are not related to this fix.
It seems easier to create new branch and open another PR.

[JavaDeveloperKiev]

Want to me create new branch and open another PR instead of this one?

Yes. I cannot merge this PR because there are commits that are not related to this fix. It seems easier to create new branch and open another PR.

I have created a new branch based on latest origin/dev to make a clean PR, but now when I try to commit I`m getting errors from the PHP Stan, not related to my changes? Any idea?

[JavaDeveloperKiev]

Same errors in GitHub desktop, not related to my changes

[JavaDeveloperKiev]

I`ve double checked, my new branch is up to date with /dev

[kenjis]

I have checked the latest develop.

$ vendor/bin/phpstan
Note: Using configuration file /Users/kenji/work/codeigniter/CodeIgniter4/phpstan.neon.dist.
 434/434 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                        
 [OK] No errors                                                                                                         

Your origin/develop is up to date?

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

[kenjis]

If you don't update Composer dependencies, run composer update.

[JavaDeveloperKiev]

If you don't update Composer dependencies, run composer update.

Worked like a charm, thank you!

Please check new PR:
#6475