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

Make it possible to resolve svg's outside \OC::$SERVERROOT #19084

Merged
merged 2 commits into from
Apr 27, 2020

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Jan 22, 2020

Resolve update blocker for snap.

Is this pr backportable? Alternatively we could keep the current approach and add the patch as fallback.

@kesselb kesselb added bug 2. developing Work in progress labels Jan 22, 2020
@kesselb kesselb force-pushed the bug/13556/wrong-paths-for-svg branch 4 times, most recently from d0a5fe0 to 875c5e1 Compare January 23, 2020 16:32
@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jan 23, 2020
@kesselb kesselb added this to the Nextcloud 19 milestone Jan 23, 2020
@kesselb kesselb marked this pull request as ready for review January 23, 2020 16:36
@rullzer
Copy link
Member

rullzer commented Jan 27, 2020

I don't get what this tries to solve...

@kesselb
Copy link
Contributor Author

kesselb commented Jan 27, 2020

I don't get what this tries to solve...

Yes description is missing some details 🙈

This PR should fix a issue with resolving the path for svg files for app directories outside the document root. It's related to #13556 but not a fix for the ResourceLocator warning.

Here are the details: #13556 (comment)

I have no idea what the current approach should do. But substr($appRootPath, strlen($this->serverRoot)) will not work if the apps directories is not a children of the server root. This will probably break other things 🤷‍♂️

If this patch is accepted it's possible to return svg's also for apps not reachable from the web. That makes no sense but was forbidden before.

I think the actual issue is somewhere else: #13556 (comment) For whatever reason symlinks are resolved at some point (probably to make other features work) but that's the actual issue. Nextcloud should work with symlinks.

@pachulo
Copy link
Contributor

pachulo commented Mar 31, 2020

Nextcloud should work with symlinks.

I think that this summarizes it quite well: for the snap to work OK, this is required.

Any news on this? Do you need more info @rullzer ?

This was referenced Apr 4, 2020
This was referenced Apr 15, 2020
@rullzer rullzer mentioned this pull request Apr 23, 2020
11 tasks
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Didn't test but the change looks good and the unit tests should prove it

Previous implementation assumes the app path is always a child \OC::$SERVERROOT. That's not always true.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb force-pushed the bug/13556/wrong-paths-for-svg branch from 875c5e1 to d766d09 Compare April 24, 2020 14:19
@juliushaertl juliushaertl added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 24, 2020
@MorrisJobke MorrisJobke merged commit 9b7e24a into master Apr 27, 2020
@MorrisJobke MorrisJobke deleted the bug/13556/wrong-paths-for-svg branch April 27, 2020 08:58
@MorrisJobke
Copy link
Member

@kesselb Backport to stable18?

@kesselb
Copy link
Contributor Author

kesselb commented Apr 27, 2020

I would prefer to not backport it ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants