Skip to content

Conversation

@gothick
Copy link
Owner

@gothick gothick commented Jan 28, 2026

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds Redis-backed caching support by introducing a Redis container for local development and configuring Symfony to use Redis as the app cache backend (via Predis).

Changes:

  • Add a redis service to docker-compose.yml with persistent storage and memory/eviction settings.
  • Switch Symfony framework.cache.app to cache.adapter.redis and set a Redis provider DSN.
  • Add predis/predis to Composer dependencies (and lockfile).

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

File Description
docker-compose.yml Adds a Redis container to support local Redis usage.
config/packages/cache.yaml Switches Symfony cache to Redis and sets the Redis DSN (including a prod override).
composer.json Adds Predis dependency required for Redis connectivity without ext-redis.
composer.lock Locks Predis and updates Composer metadata accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 11 to 15
app: cache.adapter.redis
# For dev/test environments, point to the "redis" service defined in docker-compose.yml
# We use database index 1 for application cache as on our live server WordPress uses index 0
# and it's nice to be consistent across environments.
default_redis_provider: 'redis://localhost:6379/1'
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

default_redis_provider is hard-coded to redis://localhost:6379/1. When running the Symfony app inside Docker (e.g. via make upapp, which brings up the app container), localhost refers to the PHP container itself, so the cache connection will fail even though the Redis container is running. Consider sourcing the Redis DSN from an env var and setting it to redis://redis:6379/1 for Docker (or otherwise using the redis service hostname here).

Copilot uses AI. Check for mistakes.
Comment on lines 11 to 18
app: cache.adapter.redis
# For dev/test environments, point to the "redis" service defined in docker-compose.yml
# We use database index 1 for application cache as on our live server WordPress uses index 0
# and it's nice to be consistent across environments.
default_redis_provider: 'redis://localhost:6379/1'

# APCu (not recommended with heavy random-write workloads as memory fragmentation can cause perf issues)
app: cache.adapter.apcu
# app: cache.adapter.apcu
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

This config switches the app cache to Redis for all environments, including test, but there is no when@test override (and there is no config/packages/test/cache.yaml). That makes local/CI test runs depend on a running Redis instance and will likely break tests. Add a when@test override (commonly cache.adapter.array) or keep Redis limited to dev/prod where Redis is guaranteed to be available.

Copilot uses AI. Check for mistakes.
Copy link
Owner Author

Choose a reason for hiding this comment

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

I take your point on that; I'm going to configure the containerised test environment such that it connects to the Redis server within the container correctly. I'll also configure it to use a particular database in Redis so I can double-check to make sure things are actually being cached during testing.

@gothick gothick marked this pull request as draft January 29, 2026 20:41
@gothick gothick requested a review from Copilot January 29, 2026 20:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 20 to +23
- ELASTICSEARCH_URL=http://elasticsearch:9200/
- HOST_FOR_COMMANDS=localhost
- TEST_EXIFTOOL_PATH=/usr/bin/exiftool
- REDIS_URL=redis://redis:6379/2
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

REDIS_URL is set to use database index /2, which conflicts with the cache config comment stating the app cache should use index 1. Align the DSN (and related docs) to the intended Redis DB index. Also consider adding redis under depends_on now that the app container requires it.

Copilot uses AI. Check for mistakes.
# backing off to http in some environments.
SECURE_SCHEME=https

REDIS_URL=redis://localhost:6379
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The default REDIS_URL has no database index suffix, so it will use Redis DB 0 by default. This conflicts with the cache config comment about keeping app cache on a non-zero DB index; consider setting an explicit /1 (or the intended index) here.

Suggested change
REDIS_URL=redis://localhost:6379
REDIS_URL=redis://localhost:6379/1

Copilot uses AI. Check for mistakes.
Comment on lines +11 to 12
./wait-for-it.sh $RD_HOST:$RD_PORT -t 30
./wait-for-it.sh $ES_HOST:$ES_PORT -t 120 # Elasticsearch can be very slow to get started
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The wait-for-it chain is broken: the Redis and Elasticsearch waits are on separate commands (missing && \ after the Redis wait), so a Redis timeout won’t prevent the script from continuing to the Elasticsearch wait and then migrations/apache. Chain all required waits together (or enable set -e) so the entrypoint fails fast when a dependency isn’t reachable.

Suggested change
./wait-for-it.sh $RD_HOST:$RD_PORT -t 30
./wait-for-it.sh $ES_HOST:$ES_PORT -t 120 # Elasticsearch can be very slow to get started
./wait-for-it.sh $RD_HOST:$RD_PORT -t 30 && \
./wait-for-it.sh $ES_HOST:$ES_PORT -t 120 && \ # Elasticsearch can be very slow to get started

Copilot uses AI. Check for mistakes.
# start apache
# start apache as www-data user
#exec sudo -u www-data apache2-foreground
apache2-foreground
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

apache2-foreground should be started with exec so it becomes PID 1 and receives SIGTERM/SIGINT correctly in Docker. Right now the shell remains PID 1 and signal handling/shutdown can be unreliable.

Suggested change
apache2-foreground
exec apache2-foreground

Copilot uses AI. Check for mistakes.
Comment on lines 53 to +58
RUN mkdir -p /var/www/var \
&& chown -R www-data:www-data /var/www/var \
&& mkdir -p /var/www/public/uploads \
&& chown -R www-data:www-data /var/www/public/uploads \
&& chmod 775 /var/www/public/uploads \
&& mkdir -p /var/www/public/uploads/images \
&& chown -R www-data:www-data /var/www/public/uploads/images
&& chmod 775 /var/www/public/uploads/images
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

The uploads directories are no longer owned by www-data (only chmod 775). On a fresh named volume mount, Docker copies permissions/ownership from the image; with root:root and 775 the Apache/PHP worker (www-data) typically cannot write, breaking file uploads. Restore chown (or create the dirs with www-data owner/group) and set the desired mode.

Copilot uses AI. Check for mistakes.

RUN apt-get update \
&& apt-get install -y libzip-dev git wget exiftool --no-install-recommends \
&& apt-get install -y libzip-dev git wget exiftool sudo --no-install-recommends \
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

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

sudo is added to the image, but the entrypoint doesn’t actually use it (the sudo-based exec is commented out). If Apache doesn’t need to be started via sudo, consider removing sudo to reduce image size/surface area; otherwise, switch the entrypoint to use it.

Suggested change
&& apt-get install -y libzip-dev git wget exiftool sudo --no-install-recommends \
&& apt-get install -y libzip-dev git wget exiftool --no-install-recommends \

Copilot uses AI. Check for mistakes.
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.

2 participants