add params for put interface #1365
Conversation
for later usage like nextcloud/server#22019
Codecov Report
@@ Coverage Diff @@
## master #1365 +/- ##
=========================================
Coverage 97.35% 97.35%
Complexity 2810 2810
=========================================
Files 174 174
Lines 7978 7979 +1
=========================================
+ Hits 7767 7768 +1
Misses 211 211
Continue to review full report at Codecov.
|
|
I added PHPdoc and some minor changes in an extra commit. |
|
@yrong do you still need this? |
| * | ||
| * @return string|null | ||
| */ | ||
| public function put($data); | ||
| public function put($data, $params = null); |
There was a problem hiding this comment.
this param must be optional, to prevent a BC break. 👍
There was a problem hiding this comment.
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 { …}]
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
Reverted in PR #1375
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
This is a rebase of #1290
That PR comes from the
masterbranch in https://github.com/yrong/dav and I don't want to mess with that (and maybe I can't)