-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: APCu caching driver #9874
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
base: 4.7
Are you sure you want to change the base?
Conversation
|
Hi there, sk757a! 👋 Thank you for sending this PR! We expect the following in all Pull Requests (PRs).
Important 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 do not comply with the above, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md Sincerely, the mergeable bot 🤖 |
|
The test failures are due to |
It seems to me that APCu extension just needs to be installed in the CI environment. |
Please add then in test-phpunit.yml under the extra-extensions parameter of cache-live-tests job. |
|
APCu enabled. Can't get why tests failed. |
|
Probably |
|
By default, APCu is disabled in CLI, see: https://www.php.net/manual/en/apcu.configuration.php#ini.apcu.enable-cli We can use: ini-values: apc.enable_cli=1But this should be abstracted, like other options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Please provide full details in the PR description.
This. Forgot about this parameter.
Could you elaborate a bit? At first glance, I just wanted to add
|
|
I don't think it will have any effect if you add it to the The proper way to add ini settings to setup-php is by So I think we should have an abstract value, like: CodeIgniter4/.github/workflows/reusable-phpunit-test.yml Lines 160 to 166 in 12cf2b4
|
I was in a hurry and made a mistake. The composer parameters are of course irrelevant. I rather meant something like this Thanks for the clarification. I'll look into this now. |
|
Time dependent test fail. May be just rerun? |
paulbalandan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add ext-apcu in the suggested extensions in composer.json? Both root composer.json and framework composer.json
| description: Additional PHP extensions that are needed to be enabled | ||
| type: string | ||
| required: false | ||
| extra-ini-options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change to this file should go the develop branch in a separate PR as I believe it can be used early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should make similar changes to the reusable-serviceless-phpunit-test.yml file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please
| /** | ||
| * {@inheritDoc} | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can safely remove the inheritdocs tags here.
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public function increment(string $key, int $offset = 1): bool|int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For here and decrement, you can change the return type to false|int
| /** | ||
| * {@inheritDoc} | ||
| */ | ||
| public function getCacheInfo(): array|false|object|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can limit the return type to just array|false
| if (! extension_loaded('apcu')) { | ||
| $this->markTestSkipped('APCu extension not loaded.'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use a class-level attribute #[\PHPUnit\Framework\Attributes\RequiresPhpExtension('apcu')] instead
Description
This PR enhance the framework's capabilities by adding a new caching driver based on the APCu extension.
Checklist: