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

Install missing skin via composer on system initialization #254

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bjalbor
Copy link

@bjalbor bjalbor commented Jun 7, 2024

Only classic skin ist included in core. So if you define ROUNDCUBEMAIL_SKIN to anything other, you will get an 404 (also see #243) unless you install missing skin. This contribution will install the missing skin on system startup.

@bjalbor bjalbor changed the title Install missing skin via composer Install missing skin via composer on system initialization Jun 7, 2024
@bjalbor
Copy link
Author

bjalbor commented Aug 8, 2024

Ping

@pabzm
Copy link
Member

pabzm commented Nov 7, 2024

@thomascube Do you see any technical reason not to merge this? (I'm ready to pick up the work, just want to know your opinion.)

@thomascube
Copy link
Member

@pabzm No objections. Maybe even configure a non-default skin in the docker-compose.* tests to verify the process works?

@@ -94,6 +94,20 @@ if [[ "$1" == apache2* || "$1" == php-fpm || "$1" == bin* ]]; then
${ROUNDCUBEMAIL_PLUGINS_SH};
fi

if [ ! -d skins/${ROUNDCUBEMAIL_SKIN} ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Add echo like above for plugins installation. Just to be consistent.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@pabzm
Copy link
Member

pabzm commented Nov 8, 2024

@bjalbor Could you try to extend tests/docker-compose.test-* so that they install a non-default skin, so this feature is tested?

Copy link

@pabzm
🛎️ This PR has had no activity in two weeks.

@bjalbor
Copy link
Author

bjalbor commented Nov 23, 2024 via email

@pabzm
Copy link
Member

pabzm commented Nov 25, 2024

@bjalbor No pressure, we've got time! Thank you for the notice, and thank you for working on this!

@bjalbor
Copy link
Author

bjalbor commented Nov 29, 2024

I hope everything is fine now. Sorry, if something is still missing. To be honest this is my very first contribution to a project in github 😊

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