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

Port publicpreview.php to the share 2.0 API #26611

Merged
merged 1 commit into from
Nov 21, 2016

Conversation

PVince81
Copy link
Contributor

Description

Use share 2.0 API in publicpreview.php.

Related Issue

Ref #22209 and #26608

Motivation and Context

Goal is to move away from the old API for files.

How Has This Been Tested?

Share a folder "test" with link. Put a pic in the folder.
Share a file "test.jpg" with link.
Open both links: previews appear.

Also try copying the preview links and messing with the file name, it should then return 404 for unknown or invalid files or tokens.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project. => ahem ahem...
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@DeepDiver1975 @jvillafanez

(yes I could also try and make this a controller but I'd prefer to focus on #26608 for now, PRs or additional commits welcome)

@PVince81 PVince81 added this to the 9.2 milestone Nov 11, 2016
@mention-bot
Copy link

@PVince81, thanks for your PR! By analyzing the history of the files in this pull request, we identified @georgehrke, @schiessle and @LukasReschke to be potential reviewers.

\OC_Response::setStatus(\OC_Response::STATUS_NOT_FOUND);
\OCP\Util::writeLog('core-preview', 'Passed token parameter is not valid', \OCP\Util::DEBUG);
exit;
}

if(!isset($linkedItem['uid_owner']) || !isset($linkedItem['file_source'])) {
if(is_null($linkedItem->getShareOwner()) || is_null($linkedItem->getNode())) {
Copy link
Member

Choose a reason for hiding this comment

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

We could check each item separately. Some lines below we ask again for the share owner, which is a bit awkward. There is also a check for the "getNode" function which seems that it might throw an exception, but such exception would be thrown here before.

\OC_Response::setStatus(\OC_Response::STATUS_NOT_FOUND);
\OCP\Util::writeLog('core-preview', 'Could not resolve file for shared item', \OCP\Util::WARN);
exit;
Copy link
Member

Choose a reason for hiding this comment

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

Keep going if the file / node isn't found? 😕 It would explode below with the $node->getPath()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I think I didn't mean to remove this exit

}

$pathInfo = $view->getFileInfo($path);
$sharedFile = null;
$path = $node->getPath();
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, I expect we don't need extra checks here because at this point the node should be valid and should return a valid and sane path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. If the node is not valid it would throw an exception above.

@PVince81 PVince81 force-pushed the publicpreview-share20 branch from ae9c6bf to d6300d9 Compare November 14, 2016 11:14
@PVince81
Copy link
Contributor Author

@jvillafanez fixed, I removed the extra check for the node.

Then I tested the following:

  • with deleted file but user exists: it will fail in getNode() with NotFoundException
  • with deleted user, the token cannot be resolved and it fails earlier


if($path === null) {
// FS setup and file existence check is already done in getNode()
$node = null;
Copy link
Member

Choose a reason for hiding this comment

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

This could be deleted because the variable is reassigned below or the script would finish otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted

@PVince81 PVince81 force-pushed the publicpreview-share20 branch from d6300d9 to 7d57c77 Compare November 18, 2016 17:36
@DeepDiver1975
Copy link
Member

👍

@DeepDiver1975 DeepDiver1975 merged commit 9412235 into master Nov 21, 2016
@DeepDiver1975 DeepDiver1975 deleted the publicpreview-share20 branch November 21, 2016 09:58
@lock
Copy link

lock bot commented Aug 4, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants