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

Allow specifying PHP version #223 #229

Open
wants to merge 2 commits into
base: testing
Choose a base branch
from

Conversation

fflorent
Copy link

@fflorent fflorent commented Oct 7, 2023

Problem

  • Some plugins are not compatible yet with PHP 8.2 and make wordpress crash

Solution

  • Allow the user specifying the PHP version

PR Status

  • Code finished and ready to be reviewed/tested
  • The fix/enhancement were manually tested (if applicable)

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

closes #223

@fflorent
Copy link
Author

!testme

@yunohost-bot
Copy link
Contributor

May the CI gods be with you!
Test Badge

@fflorent
Copy link
Author

!testme

@yunohost-bot
Copy link
Contributor

😜
Test Badge

Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

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

Personally, as expressed in the corresponding issue if i recall correctly, I'm against this change

This is against YunoHost's spirit : asking a technical question which ncreases the complexity of install in terms of both UI/UX and technically, equals bloat, and YunoHost is supposed to be KISS.

If some plugins are really that important then the YunoHost package should stick to some version that is compatible with those. If they aint important and are already for specific need then : YunoHost can't cover every possible need, and you people could stick to an older version of the app/package until their plugin is compatible.

Comment on lines +41 to +48
ask.en = "Choose a PHP version you want to use for your app"
ask.fr = "Choisissez une version PHP que vous souhaitez utiliser pour votre application"
type = "select"
choices = ["8.0", "8.1", "8.2"]
default = "8.2"
Copy link
Member

Choose a reason for hiding this comment

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

Or i dunno, we should at least add an help key explaining that the default value is fine for regular use case and that some plugins are only compatible with older versions of php...

@Tagadda
Copy link
Member

Tagadda commented Oct 11, 2023

I think that we shouldn't ask PHP version when installing Wordpress.
If we need to change the PHP version, I think the config panel is the good place for this setting.

@fflorent
Copy link
Author

I think we need to somehow propose the PHP version, as some plugins cannot be removed easily and still need PHP version < 8.1. Unless we are OK with downgrading the PHP version (however not sure if that's a good idea).

I like the proposal of proposing the option through the config panel, however I am not sure how simple it is to implement that (having to move the pool configuration file to a PHP version to another, reload or restart the previous and the new FPM service, removing the old dependencies and reinstalling the dependencies according to the new version, etc.).

@fflorent
Copy link
Author

fflorent commented Oct 16, 2023

Thanks for the hint @Tagadda!

App dependencies do not seem to be all reinstalled: YunoHost-Apps/my_webapp_ynh#131

I opened this PR, though I wonder if something simpler could be done: https://github.com/YunoHost-Apps/my_webapp_ynh/pull/130/files

I guess similar issue would exist for Wordpress if the php version could be changed through the config panel.

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.

Feature request: Allow specifying PHP version
4 participants