-
Notifications
You must be signed in to change notification settings - Fork 11
[ECO-5208] Support laravel 12 #59
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
Conversation
1. Increased token expiry to 8 hrs in order to reduce CPU workload 2. Updated README, updated note for defualt token expiry from 1 hr to 8 hr
WalkthroughThis pull request updates the testing matrix, dependency constraints, and token expiry configurations. The CI workflow is modified to test against newer PHP (8.3, 8.4) and Laravel (12.*) versions while excluding incompatible combinations. Dependency versions in Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
1. Updated composer to add support for laravel 12 and related testsuite dependencies 2. Updated CI workflow to run broadcaster tests with laravel 12 and supported versions of PHP
0394624 to
b2e643b
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/check.yml (1)
36-36: Remove trailing whitespace.There's a trailing whitespace on line 36 that should be removed.
- testbench: 10.* + testbench: 10.*🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 36-36: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/check.yml(3 hunks)README.md(1 hunks)composer.json(1 hunks)src/AblyBroadcaster.php(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~130-~130: Consider a shorter alternative to avoid wordiness.
Context: ...(8 hrs)** - Default expiry set to 8 hrs in order to reduce CPU workload for auth requests. ...
(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
README.md
129-129: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🪛 YAMLlint (1.35.1)
.github/workflows/check.yml
[error] 36-36: trailing spaces
(trailing-spaces)
🔇 Additional comments (8)
README.md (2)
129-130: Documentation update for token expiry settings looks good.This update clarifies the new default token expiry of 8 hours with a good explanation of the reasoning (reducing CPU workload). The added link to the GitHub issue provides valuable context for users who want to understand the decision behind this change.
🧰 Tools
🪛 LanguageTool
[style] ~130-~130: Consider a shorter alternative to avoid wordiness.
Context: ...(8 hrs)** - Default expiry set to 8 hrs in order to reduce CPU workload for auth requests. ...(IN_ORDER_TO_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
129-129: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
133-133: Example configuration update matches the new default.The updated example value of 43200 seconds (12 hours) for the environment variable is consistent with the increased default expiry time.
src/AblyBroadcaster.php (1)
30-32: Token expiry value updated to match documentation.The update of the default token expiry from 1 hour to 8 hours is properly implemented both in the default variable value and the documentation comment. This aligns with the PR objective to reduce CPU workload by minimizing token generation frequency.
.github/workflows/check.yml (3)
19-20: CI matrix expanded to include newer PHP and Laravel versions.Good update to include testing against PHP 8.3, 8.4 and Laravel 12.*. This ensures the package is properly tested against the latest versions.
34-35: Laravel 12 test configuration added correctly.The inclusion of Laravel 12.* with testbench 10.* maintains consistency with the existing pattern for other Laravel versions.
38-89: Appropriate version exclusions for incompatible combinations.The exclusions matrix has been properly updated to prevent testing Laravel 12 with incompatible PHP versions and to prevent testing older Laravel versions with newer PHP versions.
composer.json (2)
20-20: Laravel 12 support added to requirements.Good update to include Laravel 12 support in the illuminate/support dependency constraint.
24-25: Development dependencies updated for newer versions.The updates to phpunit/phpunit and orchestra/testbench version constraints are appropriate to support testing with Laravel 12.
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.
LGTM
|
@sacOO7 When can we expect this to be merged? :) |
|
@zoranlorkovic merged, we will do release soon 👍 |
|
@zoranlorkovic latest version v1.0.7 released and available to use ! |
Summary by CodeRabbit