Skip to content

Conversation

febbraro
Copy link
Member

From a bug Critter reported last week

@@ -217,7 +217,6 @@ catch_workers_output = yes
; specified at startup with the -d argument
;php_admin_value[sendmail_path] = /usr/sbin/sendmail -t -i -f www@my.domain.com
php_flag[display_errors] = off
;php_admin_value[error_log] = /opt/rh/php55/root/var/log/php-fpm/www-error.log
Copy link
Contributor

Choose a reason for hiding this comment

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

This right here... if we could get this to work with a similarly generic path, we could move the stdout symlink to base as well and not manage it as a PHP-version variation. That's aside the point of this PR, just excited to see it.

Copy link
Member Author

Choose a reason for hiding this comment

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

there is no reason we can't put that right into /var/log/php. Let's get this merged and I will review the various branches in apache-php and we can consolidate.

@@ -7,6 +7,8 @@ RUN yum -y install \

EXPOSE 80

RUN mkdir -p /var/lib/php/session
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a volume as well for session resume after a container restart?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that be a project/environment level decision to map a volume to this particular path?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm speaking of the Dockerfile VOLUME syntax, which persists data across docker runs but not docker rm.

Copy link
Member Author

Choose a reason for hiding this comment

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

@febbraro febbraro merged commit 813b52b into master Apr 11, 2017
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.

3 participants