Repository navigation
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace environment and restHost with endpoint #229
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: integration/v2
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Replace environment and restHost with endpoint #229
Changes from all commits
7b72fbcFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: ably/ably-pubsub-php
Length of output: 3263
Bracket IPv6 literals when building request URLs.
Defaults::isHostname()accepts::1, andClientOptionspreserves it as the primary domain.getHostUrl()then constructshttps://::1:443, which is not a valid cURL IPv6 authority. Bracket the URL-only value without changing the stored primary domain.Suggested fix
public function getHostUrl($host) { + if (filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) !== false) { + $host = '[' . $host . ']'; + } return ($this-> tls ? 'https://' : 'http://') . $host. ':' .$this->activePort(); }🤖 Prompt for AI Agents
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 dont need it based on ably/ably-pubsub-java#1249 (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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 8958
@sacOO7The linked discussion confirms thatDefaultsmust preserve::1unchanged, but URL builders must add brackets. The Java fix already adds those brackets in its REST and WebSocket URL builders.My PHP suggestion targets the same boundary:
ClientOptions::getHostUrl(), notDefaults::getPrimaryDomain().No change is needed in
src/Defaults.php. IfgetHostUrl()still produceshttps://::1:443, the PHP URL-formatting concern remains valid.You are interacting with an AI system.
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.
Okay, then accordingly suggest comprehensive code snippet for the same
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.
🧩 Analysis chain
🏁 Script executed:
Length of output: 10402
@sacOO7The currentgetHostUrl()concatenates the host without IPv6 brackets. Apply the formatting there, and leaveDefaultsunchanged.1. Replace
ClientOptions::getHostUrl()In
src/Models/ClientOptions.php:$urlHostis local to the method. This change does not modify the stored endpoint, primary domain, or fallback hosts.2. Add regression coverage
Add these methods inside
ClientOptionsTestintests/ClientOptionsTest.php. They use the existing PHPUnit data-provider style.These tests cover HTTP, HTTPS, default ports, custom ports, and unchanged non-IPv6 formatting. The full IPv6 case tests the URL builder directly; it does not expand the endpoint-detection contract.
I inspected the current implementation and test conventions. I have not applied this snippet or run the tests.
You are interacting with an AI system.
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: ably/ably-pubsub-php
Length of output: 6682
Reject empty or non-string
endpointvalues before resolving the primary domain.ClientOptions::$endpointis documented asstring|null, but the constructor accepts arbitrary option values and only replaces values thatempty()considers empty. A whitespace-only endpoint andnonprod:therefore reachDefaults::getPrimaryDomain()and can produce invalid domains. A non-string value such as an array reachesstrpos()and can raise a type error.Validate the endpoint at the
ClientOptionsboundary. Reject non-string values and reject routing-policy values with an empty identifier. Preserve the existing default for an unset endpoint.🤖 Prompt for AI Agents
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.