Add user-configurable defaultapp_path (fixes broken external defaultapp) #28962
Conversation
|
|
pirate
changed the title
pirate
changed the title
|
Failing build appears to be some intermittent unrelated thing, my last commit doesn't change any code and the one before that passed. |
|
Yes, you got the "600 seconds" timeout. You will need to squash all those 18 commits anyway, so I suggest that you squash and then force push the result. (I think that has to be done on a local clone of the git repo - I see you have likely been editing online in GitHub since your pull request is from your own fork and a branch called |
pirate
force-pushed
the
patch-1
branch
2 times, most recently
from
54336c5 to
7753a91
Compare
|
I started the fork online but I've been editing locally. Manual squashing isn't usually necessary, you can hit "Squash and Merge" in the UI under the merge button (unless it's not enabled in repo settings). |
| * use a comma-separated list of paths to match up with the apps in defaultapp. | ||
| * e.g. if defaultapp='external,files' defaultapp_path='/1/,/?dir=accounting' | ||
| */ | ||
| 'defaultapp_path' => '', |
There was a problem hiding this comment.
since this is a PHP file, you could even use a PHP array directly like it's done for other settings
There was a problem hiding this comment.
Cool, I wasn't sure if these options could be set by environment variable or other config file so I assumed they had to be strings, but an array is definitely better.
|
|
||
| if(getenv('front_controller_active') === 'true') { | ||
| $location = $urlGenerator->getAbsoluteURL('/apps/' . $appId . '/'); | ||
| if ($appId === 'external' && $appPath === '/') { |
There was a problem hiding this comment.
Core should not know about any external apps, please remove.
I suggest that you instead improve the external sites app to have a special route that accepts indices so that when it's opened in the browser, it will redirect to the external site page.
|
@pirate the "squash and merge" option has been disabled on this repo. They want merge commits to happen for various reasons. So PRs need to be "pre-squashed", otherwise all those 18 commits will end up in master. |
|
no need to rebase now, seems CI has passed already |
|
Unfortunately for this PR I just discovered NextCloud and ended up switching our company over a few days ago. Since the issues in this PR also exist in NC I'm in the process of rewriting this PR for NC. I might not have time to get back to this code, as I'm not even running an instance of OC to test it anymore. If anyone else wants to pick it up, I think there's only 10-30min of work left to get it working, test it, and merge it. Otherwise you're welcome to let it sit until I have some free time next week or so. |
|
Would be glad to see it on nextcloud too! |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
This adds a new user-configurable parameter called
defaultapp_path, which corresponds with the existingdefaultappand allows users to specify a custom path.This is useful if you want people to land on a certain folder or view in ownCloud upon logging in.
It's also especially useful for people who set
externalas their default app, becauseexternaldoesn't work unless a site number is specified, e.g./1/or/3/.Defaulting to a specific file or view is surprisingly useful, I'm thinking about defaulting people to our folder where we keep today's TODO list:
or
As an additional helper, if
externalis specified as thedefaultapp, but no path is given, it automatically corrects it to/1/, which is necessary to fix owncloud-archive/apps#2125.Related Issue
Fixes: owncloud-archive/apps#2125
Documentation for this change: owncloud-archive/documentation#3383
Motivation and Context
This fixes the issue reported by https://github.com/benlochard that
/external/on its own is not a valid ownCloud URL.How Has This Been Tested?
Tested partially by: https://github.com/johncab & https://github.com/MichaelBarth owncloud-archive/apps#2125 (comment)
Unit tests confirm the custom path behavior is correct.
I have not confirmed the UI behavior as I don't have a dev environment set up, (which is why this PR has so many commits, I had to keep waiting for the travis builds to finish before I knew my tests passed).
Types of changes
Checklist:
Comments:
I'm really impressed by the ownCloud code-quality so far, it seems like despite using PHP you guys managed to build a fairly clean, well-tested app that's easy to understand for a first-time contributor.
I plan to continue contributing as I find things to fix, I'm tackling owncloud-archive/apps#2216 + owncloud-archive/apps#2221 next.Sorry guys, just discovered NextCloud and ended up switching :/