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

Introduce buildPath() in Storage\Local to reduce the difference to MappedLocal #12462

Merged
merged 2 commits into from
Nov 27, 2014

Conversation

nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen added this to the 8.0-current milestone Nov 26, 2014
@nickvergessen
Copy link
Contributor Author

Could also name the method getSourcePath() to match the name of an equivalent method in the wrappers:
https://github.com/owncloud/core/pull/12426/files#diff-efea12ecdb3d8a7ad208fb446779c2a9R27

* @param bool $create
* @return string
*/
private function buildPath($path, $create = true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's $create for ? Legacy ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a parameter that exists in the mappedlocal.php implementation. It seems to be unused thou, will investigate tomorrow

@nickvergessen nickvergessen force-pushed the issue/12460-localstorage-buildpath branch from 43434bb to 07f9f43 Compare November 27, 2014 09:56
@nickvergessen
Copy link
Contributor Author

@PVince81 The parameter was never set, so I removed it and made the method in the mapper work with true

Also renamed the method as per above

* @return string
*/
private function buildPath($path, $create = true) {
private function getSourcePath($path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add PHPDoc comment that tells what this function does

@PVince81
Copy link
Contributor

Hmm, after looking at the wrapper, the question is whether we should allow wrappers to override/extend "getSourcePath()" ? If yes, that method needs to be made public. @icewind1991 what do you think ?

@nickvergessen
Copy link
Contributor Author

@PVince81 the wrapper must not overwrite it, because then the wrapper will break windows?! Oo

@nickvergessen nickvergessen force-pushed the issue/12460-localstorage-buildpath branch from 07f9f43 to 4f1bbc4 Compare November 27, 2014 10:20
@nickvergessen
Copy link
Contributor Author

Made the method protected

@scrutinizer-notifier
Copy link

The inspection completed: 13 new issues, 3 updated code elements

@ghost
Copy link

ghost commented Nov 27, 2014

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3368/
🚀 Test PASSed. 🚀

@icewind1991
Copy link
Contributor

Wrappers shouldn't overwrite the getSourcePath, a wrapper has no knowledge over the wrapped storage and uses parent::/$this->storage-> calls to do their work

@icewind1991
Copy link
Contributor

👍 code looks good

@PVince81
Copy link
Contributor

Did not destroy the known universe 👍

PVince81 pushed a commit that referenced this pull request Nov 27, 2014
…dpath

Introduce buildPath() in Storage\Local to reduce the difference to MappedLocal
@PVince81 PVince81 merged commit e733d32 into master Nov 27, 2014
@PVince81 PVince81 deleted the issue/12460-localstorage-buildpath branch November 27, 2014 13:57
@lock lock bot locked as resolved and limited conversation to collaborators Aug 15, 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