feat: Add lib_name and client_info_tag to client configuration - #1
Jonathan-Improving wants to merge 3 commits into
Conversation
Add two new parameters to the ValkeyGlide and ValkeyGlideCluster connect() methods: - lib_name: Full override for the CLIENT SETINFO LIB-NAME value (default: 'GlidePHP' set by Rust core when NULL) - client_info_tag: Tag appended as 'GlidePHP(tag)' for framework attribution without losing GLIDE identity Behavior matrix: | Config | Resulting lib-name | |-------------------------------------|-----------------------------| | (none) | GlidePHP (Rust core default)| | client_info_tag='fw:1.0' | GlidePHP(fw:1.0) | | lib_name='custom' | custom | | lib_name='custom', tag='fw:1.0' | custom(fw:1.0) | Validation: client_info_tag rejects whitespace (space, tab, newline, CR) with ValkeyGlideException. Passes lib_name to ConnectionRequest protobuf field 19, consumed by Rust core for CLIENT SETINFO LIB-NAME during connection establishment. Includes unit tests (ConnectionRequestTest) and integration tests (ValkeyGlideFeaturesTest). Refs: valkey-io/valkey-glide#6389 (Python reference implementation) Signed-off-by: extollIT Enterprises <support@extollit.com>
MatthiasHowellYopp
left a comment
MatthiasHowellYopp
left a comment
There was a problem hiding this comment.
APPROVE (with nits)
Clean, well-implemented PR. The C extension code is memory-safe, parameter handling is correct, and the implementation aligns with the upstream Python reference (valkey-io/valkey-glide#6389). No blocking issues.
ℹ️ CI not triggered — no checks reported on this branch.
|
|
||
| public function testConstructorWithSingleAddress() | ||
| { | ||
| // Test constructor with single address in proper array format |
There was a problem hiding this comment.
The integration tests here only exercise the standalone ValkeyGlide client. The ValkeyGlideCluster.__construct also accepts lib_name/client_info_tag, but there's no end-to-end test verifying CLIENT INFO output for the cluster client.
The unit tests in ConnectionRequestTest.php cover protobuf serialization for both paths, so this isn't blocking — but consider adding at least one cluster integration test (e.g., in ValkeyGlideClusterFeaturesTest.php) before pushing upstream.
| @@ -120,6 +122,21 @@ PHP_METHOD(ClientConstructorMock, simulate_standalone_constructor) { | |||
| return; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
💅 Nit: Double blank line before the validation block. The rest of the file uses single blank lines between logical sections.
| /* Validate client_info_tag contains no whitespace */ |
| conn_req.client_name = config->client_name ? config->client_name : NULL; | ||
|
|
||
| /* Set lib_name: compose from lib_name override and/or client_info_tag */ | ||
| char* composed_lib_name = NULL; |
There was a problem hiding this comment.
💅 Nit (edge case): The composition logic uses if (config->lib_name && config->client_info_tag) which is truthy for an empty string "" (non-NULL pointer). With lib_name="" + client_info_tag="foo", the result would be "(foo)" — a bare open paren with no prefix.
Unlikely in practice (callers pass null for unset), and the Python reference doesn't guard against this either. Just documenting — no action needed unless you want to add a strlen > 0 check for symmetry.
- F-PHP-001: Replace manual char checks with isspace() covering \v and \f - F-PHP-002: Extract validate_client_info_tag() into common.h, called by all 4 sites Signed-off-by: Jonathan Neufeld <jonathan.neufeld@improving.com>
Signed-off-by: Jonathan Neufeld <jonathan.neufeld@improving.com>
|
Superseded by valkey-io#292 |
3 participants
Internal Review
Soliciting internal review before finalizing the upstream draft PR.
Upstream draft: valkey-io#292
Jira: AEA-632
Parent issue: valkey-io/valkey-glide#6429
Summary
Adds
lib_nameandclient_info_tagparameters to the PHP GLIDE client constructor. Whenclient_info_tagis set, it appends a parenthesized tag to the library name sent viaCLIENT SETINFO LIB-NAME(e.g.GlidePHP(my-framework:1.0)).Behavior
GlidePHP(default)client_info_tag="foo"GlidePHP(foo)lib_name="custom"customlib_name="custom" + client_info_tag="foo"custom(foo)Implementation notes
client_namepattern through FFIReview focus
Commits will likely be squashed before pushing upstream.