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

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

Closed
geerlingguy opened this issue Mar 29, 2017 · 4 comments
Closed

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

geerlingguy opened this issue Mar 29, 2017 · 4 comments

Comments

@geerlingguy
Copy link
Owner

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
Copy link
Contributor

oxyc commented Apr 11, 2017

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
Copy link
Owner Author

@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
Copy link
Contributor

mglaman commented Apr 19, 2017

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
Copy link
Owner Author

Done! Thanks @mglaman

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

No branches or pull requests

3 participants