-
Notifications
You must be signed in to change notification settings - Fork 699
[5.x] Fixes #1616
#1617
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
[5.x] Fixes #1616
#1617
Conversation
|
This does not fix the problem of using a 'horizon' connection. I don't get the point of this PR and the one that introduced this check. Horizon config by default uses the redis connection 'default'. Why can't we add a 'horizon' connection and use that one instead of the 'default' one so we can use another redis database to store horizon data? The code adds/overrides the 'prefix' option but uses the other settings as it would do with the 'default' connection. I may not want the horizon data on the redis db 0 mixed with other things... that's why i created the 'horizon' connection. |
|
In fact, this is a validation that was created according to the documentation, and it’s better not to use a connection with However, the main issue is related to cached previous configurations that conflict with the current validation. |
|
Documentation states that 'it should not' be used. But it's not mandatory to use another connection other than 'horizon'. You can provide any connection in 'use' and the code will overwrite any fields it needs. That's why there's a warning, so you know some database parameters could be overriden, in this case, only 'prefix' is changed. I think the original #1615 should be reverted, and allow it as it's been until now, because it's a breaking change as it's not mandatory to use another connection name. |
|
This breaks |
|
So this whole thing started because of this part in the docs? 🤯
Things are in the docs as they aren't in code. And Laravel was never restrictive about any assumptions. You are just f*ed and on your own when you play different. I don't get why this was put in in the first place and even less why it's not simply and fully reverted? Let me do what I want. When checking the issues I couldn't find a single one that hit this problem @amirhshokri tried to fix with that!? So why do we try to solve an issue that doesn't exist and even when it would exist, it's clearly documented. |
Fixes #1616