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.

Sign up for GitHub

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

add params for put interface #1365

Merged
merged 4 commits into from
Nov 30, 2021
Merged

add params for put interface #1365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6432caa
add params for put interface
yrong Aug 11, 2020
a4432fa
php cs fix
yrong Aug 11, 2020
0e2b4e3
php unit fix
yrong Aug 11, 2020
2df6a6c
Add PHPdoc for params parameter
phil-davis Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/CalDAV/CalendarObject.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,11 @@ public function get()
* Updates the ICalendar-formatted object.
*
* @param string|resource $calendarData
* @param object|null $params
*
* @return string
*/
public function put($calendarData)
public function put($calendarData, $params = null)
{
if (is_resource($calendarData)) {
$calendarData = stream_get_contents($calendarData);
Expand Down
3 changes: 2 additions & 1 deletion lib/CalDAV/Schedule/SchedulingObject.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,11 @@ public function get()
* Updates the ICalendar-formatted object.
*
* @param string|resource $calendarData
* @param object|null $params
*
* @return string
*/
public function put($calendarData)
public function put($calendarData, $params = null)
{
throw new MethodNotAllowed('Updating scheduling objects is not supported');
}
Expand Down
5 changes: 3 additions & 2 deletions lib/CardDAV/Card.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,12 @@ public function get()
/**
* Updates the VCard-formatted object.
*
* @param string $cardData
* @param string $cardData
* @param object|null $params
*
* @return string|null
*/
public function put($cardData)
public function put($cardData, $params = null)
{
if (is_resource($cardData)) {
$cardData = stream_get_contents($cardData);
Expand Down
3 changes: 2 additions & 1 deletion lib/DAV/CorePlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,7 @@ public function httpPut(RequestInterface $request, ResponseInterface $response)
{
$body = $request->getBodyAsStream();
$path = $request->getPath();
$params = (object) ['versioning' => $request->getHeader('versioning')];

// Intercepting Content-Range
if ($request->getHeader('Content-Range')) {
Expand Down Expand Up @@ -489,7 +490,7 @@ public function httpPut(RequestInterface $request, ResponseInterface $response)
if (!($node instanceof IFile)) {
throw new Exception\Conflict('PUT is not allowed on non-files.');
}
if (!$this->server->updateFile($path, $body, $etag)) {
if (!$this->server->updateFile($path, $body, $etag, $params)) {
return false;
}

Expand Down
5 changes: 3 additions & 2 deletions lib/DAV/FS/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ class File extends Node implements DAV\IFile
/**
* Updates the data.
*
* @param resource $data
* @param resource $data
* @param object|null $params
*/
public function put($data)
public function put($data, $params = null)
{
file_put_contents($this->path, $data);
clearstatcache(true, $this->path);
Expand Down
3 changes: 2 additions & 1 deletion lib/DAV/FSExt/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,11 @@ class File extends Node implements DAV\PartialUpdate\IPatchSupport
* Data is a readable stream resource.
*
* @param resource|string $data
* @param object|null $params
*
* @return string
*/
public function put($data)
public function put($data, $params = null)
{
file_put_contents($this->path, $data);
clearstatcache(true, $this->path);
Expand Down
3 changes: 2 additions & 1 deletion lib/DAV/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,11 @@ abstract class File extends Node implements IFile
* return an ETag, and just return null.
*
* @param string|resource $data
* @param object|null $params
*
* @return string|null
*/
public function put($data)
public function put($data, $params = null)
{
throw new Exception\Forbidden('Permission denied to change data');
}
Expand Down
3 changes: 2 additions & 1 deletion lib/DAV/IFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,11 @@ interface IFile extends INode
* return an ETag, and just return null.
*
* @param resource|string $data
* @param object|null $params
*
* @return string|null
*/
public function put($data);
public function put($data, $params = 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 param must be optional, to prevent a BC break. 👍

Copy link

Choose a reason for hiding this comment

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

this still is a bc break: [php] Fatal Compile Error: Declaration of Pimcore\Model\Asset\WebDAV\File::put($data) must be compatible with Sabre\DAV\File::put($data, $params = null) ["exception" => Symfony\Component\ErrorHandler\Error\FatalError { …}]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's annoying!
It can nearly work because put($data) can be happily written when calling an instance of the parent and/or the child class. If PHP 7.2+ didn't check this, then actually this code could still work.

But if someone writes new code that calls an instance a IFile then they should be able to write put($data, $params) and pass the new parameter, if they wish. But then at run-time the object might be an instance of the child class, and that object will not know anything about the 2nd parameter. But the child should function as a fully-working drop-in replacement for the parent class - it doesn't.

Therefore this change is dangerous for future code (but could have worked for everyone's existing code, if PHP "let it past").

A good explanation of this stuff is in https://platform.sh/blog/watch-your-inheritance/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted in PR #1375

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert is released in https://github.com/sabre-io/dav/releases/tag/4.2.3

@dpfaffenbauer thanks for reporting! I will ask if that extra parameter to put($data) is really a needed thing. If it is, then we need to think if there is any BC way to implement it, or if it would have to be part of a major version.

Copy link

@dpfaffenbauer dpfaffenbauer Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot for reacting so fast :) I owe you a beer now.

Regarding BC, add the new param like this /* $params = null */ and use func_get_args to check if there is a second param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for reacting so fast :) I owe you a beer now.

Actually, I owe you a beer for reporting this quickly, so that it could be reverted before too many people bump their dependencies.


/**
* Returns the data.
Expand Down
12 changes: 6 additions & 6 deletions lib/DAV/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1114,13 +1114,14 @@ public function createFile($uri, $data, &$etag = null)
*
* This method will return true if the file was actually updated
*
* @param string $uri
* @param resource $data
* @param string $etag
* @param string $uri
* @param resource $data
* @param string $etag
* @param object|null $params
*
* @return bool
*/
public function updateFile($uri, $data, &$etag = null)
public function updateFile($uri, $data, &$etag = null, $params = null)
{
$node = $this->tree->getNodeForPath($uri);

Expand All @@ -1133,8 +1134,7 @@ public function updateFile($uri, $data, &$etag = null)
if (!$this->emit('beforeWriteContent', [$uri, $node, &$data, &$modified])) {
return false;
}

$etag = $node->put($data);
$etag = $node->put($data, $params);
if ($modified) {
$etag = null;
}
Expand Down
5 changes: 3 additions & 2 deletions tests/Sabre/DAV/Mock/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,12 @@ public function setName($name)
* different object on a subsequent GET you are strongly recommended to not
* return an ETag, and just return null.
*
* @param resource $data
* @param resource $data
* @param object|null $params
*
* @return string|null
*/
public function put($data)
public function put($data, $params = null)
{
if (is_resource($data)) {
$data = stream_get_contents($data);
Expand Down
5 changes: 3 additions & 2 deletions tests/Sabre/DAV/Mock/StreamingFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,12 @@ class StreamingFile extends File
* different object on a subsequent GET you are strongly recommended to not
* return an ETag, and just return null.
*
* @param resource $data
* @param resource $data
* @param object|null $params
*
* @return string|null
*/
public function put($data)
public function put($data, $params = null)
{
if (is_string($data)) {
$stream = fopen('php://memory', 'r+');
Expand Down
8 changes: 4 additions & 4 deletions tests/Sabre/DAV/PartialUpdate/FileMock.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ class FileMock implements IPatchSupport
{
protected $data = '';

public function put($str)
public function put($data, $params = null)
{
if (is_resource($str)) {
$str = stream_get_contents($str);
if (is_resource($data)) {
$data = stream_get_contents($data);
}
$this->data = $str;
$this->data = $data;
}

/**
Expand Down