geerlingguy / ansible-role-drupal

Ansible Role - Drupal
https://galaxy.ansible.com/geerlingguy/drupal/
MIT License
92 stars 51 forks source link

Get rid of soft dependency on geerlingguy.php role #35

Closed geerlingguy closed 7 years ago

geerlingguy commented 7 years ago

Currently this role calls the restart webserver handler in the geerlingguy.php role when a Drupal deployment is completed, but this role shouldn't, since that role is not a hard dependency.

In the PHP role, the handlers defined are:

- name: restart webserver
  service:
    name: "{{ php_webserver_daemon }}"
    state: restarted
  notify: restart php-fpm
  when: php_enable_webserver

- name: restart php-fpm
  service:
    name: "{{ php_fpm_daemon }}"
    state: restarted
  when: php_enable_php_fpm

We could either define our own handler(s), or figure out whether it's strictly necessary or not. I think the reason I had that in there initially was for the Raspberry Pi-based setup I tested on—I had opcache configured to not stat files on every page load since the files were on slow microSD cards... for most servers (and by default), since opcache is looking at each file, we technically don't need a hard restart of php-fpm to pick up changed files.

oxyc commented 7 years ago

I'd vote for just dropping the restart. I'd say the users requiring the opcache to clear is below the <20% use case and could just opt for a custom script and #34. Personally I do https://github.com/generoi/capistrano-tasks/blob/master/lib/capistrano/tasks/cache.rake#L42:L69

geerlingguy commented 7 years ago

@oxyc +1 to that; and I don't think we need to block getting this fixed on #34 (though that one shouldn't be terribly difficult to fix either).

mglaman commented 7 years ago

I'd say it's fine to drop the restart. Especially if we just document the issue, or do as suggested by @oxyc. Or instead of making a file, minify the line and pass to drush eval to execute clearing it.

So

drush eval "if (function_exists('apc_clear_cache')) {apc_clear_cache();}if (function_exists('opcache_reset')) {opcache_reset();}"

Tested this locally and seemed to work okay.

geerlingguy commented 7 years ago

Done! Thanks @mglaman