Skip to content

Add parameter opt out for setinfo commands#1370

Open
kylekatarnls wants to merge 12 commits into
predis:v2.xfrom
kylekatarnls:feature/setinfo-opt-out
Open

Add parameter opt out for setinfo commands#1370
kylekatarnls wants to merge 12 commits into
predis:v2.xfrom
kylekatarnls:feature/setinfo-opt-out

Conversation

@kylekatarnls

@kylekatarnls kylekatarnls commented Aug 24, 2023

Copy link
Copy Markdown

This gives an opt-out for users who don't need the bootstrap SETINFO and want to remove them for performance or other reasons.

@kylekatarnls kylekatarnls force-pushed the feature/setinfo-opt-out branch from 7a83390 to 7f9fd68 Compare August 24, 2023 09:34

@okhoshi okhoshi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems overkill to have 2 parameters to control this

Comment thread src/Connection/Factory.php Outdated
@kylekatarnls

Copy link
Copy Markdown
Author

Fix for #1347 (comment)

Comment thread src/Connection/ParametersInterface.php Outdated
tillkruss and others added 2 commits August 24, 2023 11:27
Co-authored-by: Quentin D. <4972091+Okhoshi@users.noreply.github.com>
@tillkruss tillkruss self-assigned this Aug 24, 2023
tillkruss
tillkruss previously approved these changes Aug 24, 2023
Comment thread src/Connection/Factory.php Outdated
@kylekatarnls

Copy link
Copy Markdown
Author

Trimmed the whitespace eclint complained about:
https://github.com/predis/predis/actions/runs/5967543294/job/16189493022?pr=1370#step:6:17

@kylekatarnls kylekatarnls requested a review from tillkruss August 24, 2023 18:55
tillkruss
tillkruss previously approved these changes Aug 24, 2023
@vladvildanov

Copy link
Copy Markdown
Contributor

Test coverage?

@kylekatarnls

Copy link
Copy Markdown
Author

Test added. You need to "Approve and run workflows" to trigger tests and get result from coveralls

@kylekatarnls

Copy link
Copy Markdown
Author

Redis 7 errors unrelated (might have been introduced by last week update of Redis 7.2)

Can be verified with #1372

Comment thread tests/Predis/Connection/FactoryTest.php
@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 78.183% (+0.02%) from 78.167% when pulling 5cb1702 on kylekatarnls:feature/setinfo-opt-out into 16bdd83 on predis:v2.x.

@vladvildanov vladvildanov requested review from BafS, chayim and okhoshi and removed request for BafS and okhoshi August 28, 2023 11:18
Comment thread src/Connection/Parameters.php

@tillkruss tillkruss left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify client_* options.

@vladvildanov

Copy link
Copy Markdown
Contributor

@tillkruss

Copy link
Copy Markdown
Member

No, why do we need client name/version to be configurable at all? It should always be Predis, no?

@vladvildanov

Copy link
Copy Markdown
Contributor

@tillkruss I believe @chayim knows more than me about that. It looks like clients should be able to advertise their own products this way. F.E if magento uses predis as Redis client, they can override this values, so cloud instances can collect this kind of data about magento clients.

@chayim

chayim commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

The reason for allowing setting names and versions is to enable "whatever people want to create". It's entirely possible to build client solutions on top of existing clients (nredisstack and stackexchange.redis for example). This is the vision of how these things would be used.

It also allows (same example) for a client to advertise something like:
Name: Foo [predis]

Thus, sharing all.

@tillkruss

Copy link
Copy Markdown
Member

@kylekatarnls: I'll merge a quick fix in #1399, because #1398 is making this more complicated.

@tillkruss tillkruss removed their assignment Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

7 participants