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

Fix parameter preparation for occ command #518

Merged
merged 1 commit into from
Mar 4, 2025
Merged

Conversation

R0Wi
Copy link
Member

@R0Wi R0Wi commented Feb 17, 2025

Notes

  • I was only able to reproduce this on NC31 (because I didn't find any NC32 compatible apps in the store, yet). So the base for this fix is stable31
  • I took the liberty of defaulting $this->config->getSystemValue('apps_paths') with [], because I found some errors in my logs when trying to uninstall an app (something like "foreach needs an array, string given ..."). This happens if app_paths is not set. getSystemValue will return an "empty" string in that case.
  • If possible, a testcase should be added for this. Maybe with any deployed app which is updated regularly? Note that the problem did not occur when doing the "test install"

@R0Wi R0Wi requested a review from oleksandr-nc February 17, 2025 20:43
@R0Wi R0Wi linked an issue Feb 17, 2025 that may be closed by this pull request
Copy link
Contributor

@oleksandr-nc oleksandr-nc left a comment

Choose a reason for hiding this comment

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

Was on vacation, sorry for the delay in replying.

And why is PR made in stable31 branch? Maybe it would be better to make it in main and backport it?

@oleksandr-nc
Copy link
Contributor

@R0Wi good time of day. Are you willing to rebase this PR on main branch or should we do it ourselves?

@R0Wi
Copy link
Member Author

R0Wi commented Mar 3, 2025

Hi @oleksandr-nc, sorry for the delayed answer. Unfortunately I've been out sick last week.

Happy to incorporate your review findings tonight 👍

* Fix for #517

Signed-off-by: Robin Windey <ro.windey@gmail.com>
@R0Wi R0Wi force-pushed the fix/appstore-install branch from 275486f to 85d7934 Compare March 3, 2025 16:21
@R0Wi R0Wi changed the base branch from stable31 to main March 3, 2025 16:22
@R0Wi
Copy link
Member Author

R0Wi commented Mar 3, 2025

@oleksandr-nc rebased on main and incorporated your feedback. Needs to be backported to stable31 ...

@oleksandr-nc oleksandr-nc requested review from kyteinsky and removed request for bigcat88 March 3, 2025 17:03
@oleksandr-nc oleksandr-nc merged commit 71479b3 into main Mar 4, 2025
1 check passed
@oleksandr-nc oleksandr-nc deleted the fix/appstore-install branch March 4, 2025 07:17
@oleksandr-nc
Copy link
Contributor

/backport to stable31

@oleksandr-nc
Copy link
Contributor

@oleksandr-nc rebased on main and incorporated your feedback. Needs to be backported to stable31 ...

Thank you

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.

[bug] Issue when trying to install external app via appstore
3 participants