fix(files): properly forward open params from short urls#50807
fix(files): properly forward open params from short urls#50807
Conversation
skjnldsv
force-pushed
the
fix/redirect-openfile-param
branch
from
93c4754 to
0aeb29e
Compare
|
/backport to stable31 |
|
/backport to stable30 |
|
/backport to stable29 |
skjnldsv
force-pushed
the
fix/redirect-openfile-param
branch
from
0aeb29e to
adcd9b9
Compare
skjnldsv
force-pushed
the
fix/redirect-openfile-param
branch
from
adcd9b9 to
70394e0
Compare
| $this->useCollection('root'); | ||
| $this->setupRoutes($this->getAttributeRoutes('core'), 'core'); | ||
| require_once __DIR__ . '/../../../core/routes.php'; | ||
| require __DIR__ . '/../../../core/routes.php'; |
There was a problem hiding this comment.
This is an important point.
After a debug session with Christoph, we realised that it prevented routes.php files to be included again when running tests. Making any subsequent route generation fail.
Considering we already test that app have been loaded with isset($this->loadedApps['core']) and isset($this->loadedApps[$app]), we assumed it was safe to require/include without _once.
skjnldsv
force-pushed
the
fix/redirect-openfile-param
branch
2 times, most recently
from
d4e9a9c to
93fc392
Compare
skjnldsv
requested review from
a team,
ArtificialOwl,
ChristophWurst,
come-nc,
icewind1991 and
susnux
and removed request for
a team
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
Not fully sure as the opendetails is a new feature. Also I do not see any reason for neither having one of them for this endpoint.
Meaning: if openfile is false set opendetails, setting both or none makes no sense.
I don't understand this sentence. |
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
I agree with susnux comments about adding typing.
The logic around opendetails/openfile looks solid, so my only blocker here is this fileNotFound situation.
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
skjnldsv
force-pushed
the
fix/redirect-openfile-param
branch
from
93fc392 to
66c8e70
Compare
Addressed and added dedicated cypress tests! |
Not sure about all apps (if you can control the sidebar when the file is opened), but if thats the case then maybe. But my point was more like: That "internal link" feature make no sense with
Meaning there is no need for That beeing said its no blocker from my side, having this new feature (parsing `opendetails´ on the internal-link endpoint) just makes no sense to me, but maybe I am overlooking something here 😅 |
| #[NoAdminRequired] | ||
| #[NoCSRFRequired] | ||
| public function showFile(?string $fileid = null): Response { | ||
| public function showFile(?string $fileid = null, ?string $opendetails = null, ?string $openfile = null): Response { |
There was a problem hiding this comment.
What I meant is adding openfile here is a bugfix ✅
Adding opendetails is a feature I do not see the usecase on this endpoint.
There was a problem hiding this comment.
Ah right, it's kinda both you're right!
I'll adjust the backports to only cover the openfile anyway :)
Ah, but this is needed, because otherwise the opendetails will be stripped from the URL. It's not about being a bugfix or not, it's mainly like: "since we're here, better do it properly and make it feature complete" :) |
3 participants
Fix #50155