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

[fix] php conf files not properly removed when an app is uninstalled #566

Merged
merged 1 commit into from Nov 4, 2018

Conversation

Projects
None yet
3 participants
@alexAubin
Member

alexAubin commented Oct 25, 2018

The problem

As reported here YunoHost/issues#1213, here https://blog.genma.fr/?Yunohost-Soucis-a-la-suppression-d-une-webapp and in several posts on the forum, we currently have an important bug with PHP apps where a file is not removed and makes php7.0-fpm crashes.

Solution

Turns out ynh_remove_fpm_config still assumed we were in jessie with Php5 :|

PR Status

Tested and ready

How to test

Follow Genma's instructions with and without the PR : https://blog.genma.fr/?Yunohost-Soucis-a-la-suppression-d-une-webapp

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 25, 2018

(As it's an important bug, I propose to merge this soon, and maybe push a hotfix on 3.2.x)

@alexAubin alexAubin changed the title from We are in Stretch and use php7 now to [fix] php conf files not properly removed when an app is uninstalled Oct 25, 2018

@maniackcrudelis

This comment has been minimized.

Contributor

maniackcrudelis commented Oct 25, 2018

The remove helper should remove both php5 and 7 files.
Let's add a ynh_exec_warn_less to avoid any unneeded warnings.

@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 25, 2018

Uh but there are no php5 file anyway, are there ...?

@maniackcrudelis

This comment has been minimized.

Contributor

maniackcrudelis commented Oct 25, 2018

I misread your PR.
You want to assume that there's no php5 files anymore.
Well, sure, it's more consistent with the other helper.
But are we sure, during the migration and in case of restore we don't have any php5 files left ?

Looks like it's not so obvious, regarding all the bugs we have about php.
But maybe it's only about this very case. I didn't take time to read carefully all the cases

@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 25, 2018

Weeell what I can say is that during the migration to stretch, all php5 pool conf file are automatically moved to php7 pool conf files, and stuff like nginx conf files are also edited to use php7 socket.

So no PHP5 stuff should be "active" anymore, though conf file might still be there. But the php5-fpm service should not even be started (if it's still installed), let alone being used by anything..

@maniackcrudelis

This comment has been minimized.

Contributor

maniackcrudelis commented Oct 25, 2018

OK, looks good to me then.
And what if the path in settings.yml is on php5 ? Is it even possible ?

@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 25, 2018

No idea :D I guess that's possible yes, if an app absolutely wanted to use PHP5 for some reason and knows how to handle it properly ?

@maniackcrudelis

This comment has been minimized.

Contributor

maniackcrudelis commented Oct 25, 2018

My question was more about that, 8590d6b
So the answer is no.

EDIT: Actually, looks like it was merge in jessie as well, https://github.com/YunoHost/yunohost/blob/stable/data/helpers.d/backend
So, yes an app could have store the php5 path of its config in settings.yml

@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 25, 2018

Hm I didnt notice those lines from the helper that add the config :

ynh_app_setting_set $app fpm_config_dir "$fpm_config_dir"
ynh_app_setting_set $app fpm_service "$fpm_service"

Now I don't understand anymore why it wasnt working (in the context of installing then removing the app on Stretch as described in Genma's post) ... but if I understand correctly, this could also happen that this var is not empty because defined while being on Jessie ?

If so, I don't know what's the proper fix except something like :

if (fpm_config_dir is empty) or (fpm_config_dir == "/etc/php5/fpm")
then
    fpm_config_dir = "/etc/php/7.0/fpm"
endif
@maniackcrudelis

This comment has been minimized.

Contributor

maniackcrudelis commented Oct 25, 2018

Hum, since it's a remove helper and not removing a config file could create an error.
I think removing all possible config file would be the safest way to do it.

@alexAubin

This comment has been minimized.

Member

alexAubin commented Nov 4, 2018

Meh I don't understand the other part of the bug related to old apps from a jessie install (c.f. the fpm_config_dir in 8590d6b ) and too tired to dig this ... but at least the current PR should fix the issue of new app install in stretch as described by Genma, so merging this for now...

@alexAubin alexAubin merged commit 787bfaa into stretch-unstable Nov 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the fix-1213-php-files-not-properly-removed branch Nov 4, 2018

@alexAubin alexAubin added this to the 3.3.x milestone Nov 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment