Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Addshuffle env processor#46883
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ro0NL 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.
IIUC you could seedshuffle usingmt_srand in tests
Uh oh!
There was an error while loading.Please reload this page.
4c15709 tod18d383Compare
nicolas-grekas 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.
works for me, here are some comments
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
2caa9e5 toa921ed4Compare94noni commentedJul 10, 2022
Can/will this be chainable with the « env(csv:FOO) » env processor? |
ostrolucky commentedJul 10, 2022
of course |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedJul 10, 2022
Thank you@ostrolucky. |
| } | ||
| if ('shuffle' ===$prefix) { | ||
| \is_array($env) ?shuffle($env) :thrownewRuntimeException(sprintf('Env var "%s" cannot be shuffled, expected array, got "%s".',$name,get_debug_type($env))); |
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.
IMHO this is "abusing" ternary, the more "classical"
if (!\is_array($env)) {thrownewRuntimeException(sprintf('Env var "%s" cannot be shuffled, expected array, got "%s".',$name,get_debug_type($env))); }shuffle($env);
would be better
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.
Subjective
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.
Indeed, hence "IMHO" 😉 but I can try "more objective" arguments:
- it's not using the expression result (BTW: shuffle() always returns true [historically could also return false], and if designed today would probably be
: void; andthrowwasn't usable as/in an expression before PHP 8.0) - it's inconsistent with all the rest of the file (even the general codebase)
Dunno if the CS tool could flag it?
Uh oh!
There was an error while loading.Please reload this page.
Motivation of this: We are specifying list of hosts for Redis cluster as env var. The way redis library unfortunately works is that it always takes first host and tries to connect to it (and after initial connection is done successfully, only then jump to other nodes). This makes first host under load much more than others. This env var processor could be used to solve it and distribute the load better.