Skip to content
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

Guarantee REDIS_HOST_PORT is never null for UNIX sockets #1968

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshtrichards
Copy link
Member

Addresses #1785 (which arises when using UNIX sockets)

Without this fix, UNIX socket connections won't work if REDIS_HOST is used to specify the socket to the container unless an additional bogus REDIS_HOST_PORT is also specified.

This change makes the auto-generated REDIS socket configuration in the Docker container (which overrides whatever is specified in config.php whenver REDIS_HOST is used) match NC's recommended default (which includes specifying port 0 for sockets). More importantly it's more user friendly and eliminates non-working setups with error messages such as the following (which results in an Internal Server Error):

Deprecated: Redis::pconnect(): Passing null to parameter #2 ($port) of type int is deprecated in /var/www/html/lib/private/RedisFactory.php on line 137

P.S. I believe this is the only file in the docker repository that needs to be updated manually - my reading is that update.sh is used as part of the release process and will update the ones for each version/config model appropriately. If I'm incorrect let me know and I'll update those too. Should all be the same.

Addresses nextcloud#1785 (which arises when using UNIX sockets)

Without this fix (or manually specifying a bogus REDIS_HOST_PORT) UNIX socket connections won't work if REDIS_HOST is used to specify the socket to the container. 

Signed-off-by: Josh R. <[email protected]>
@PhrozenByte
Copy link
Contributor

We should probably rather fix this in nextcloud/server and drop $CONFIG['redis']['port'] if the user specifies no port, simply because default values should be applied at a single place (and nextcloud/server already defaults to port 6379 if no port is configured).

So, can you please open a PR against nextcloud/server setting $port = 0 as default in lib/private/RedisFactory.php and then change this PR to drop $CONFIG['redis']['port'] if the user specifies no port? Please don't forget to sign-off your commits (git commit -s, see here) and please also run ./update.sh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants