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

Simplify isValidPath and add unit tests #13224

Merged
merged 1 commit into from Jan 10, 2015

Conversation

Projects
None yet
6 participants
@LukasReschke
Copy link
Member

LukasReschke commented Jan 9, 2015

The check for invalid paths is actually over-complicated and performed with the not really performant strstr resulting in a performance penalty. Additionally, I decided to add unit-tests to that function.

Part of #13221

@icewind1991 Your review would be appreciated.

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Jan 9, 2015

👎 What about paths that contain /.. but don't reference the parent subdir, like /foo/..bar ? A perfectly valid path, but one that would be caught out by this code. Perhaps the unit tests should catch that?

Other than that, the unit tests are a nice addition.

@LukasReschke

This comment has been minimized.

Copy link
Member Author

LukasReschke commented Jan 9, 2015

👎 What about paths that contain /.. but don't reference the parent subdir, like /foo/..bar ? A perfectly valid path, but one that would be caught out by this code. Perhaps the unit tests should catch that?

Well - they failed before as well ;-)

@LukasReschke

This comment has been minimized.

Copy link
Member Author

LukasReschke commented Jan 9, 2015

Ah damn – got it now – with a word after it they work… Mhm…

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Jan 9, 2015

Personally I like the heavyweight approach of explode()ing the string at / and checking each component. But I understand it's perhaps not the most performant code 😆

@LukasReschke LukasReschke force-pushed the simplify-is-valid-path-and-add-unit-tests branch Jan 9, 2015

@LukasReschke

This comment has been minimized.

Copy link
Member Author

LukasReschke commented Jan 9, 2015

@Xenopathic Adjusted. - Great catch. - Added unit tests for that as well and switched to the faster strpos

Simplify isValidPath and add unit tests
The check for invalid paths is actually over-complicated and performed twice resulting in a performance penalty. Additionally, I decided to add unit-tests to that function.

Part of #13221

@LukasReschke LukasReschke force-pushed the simplify-is-valid-path-and-add-unit-tests branch to 05615bf Jan 9, 2015

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Jan 9, 2015

👍 😄

@Xenopathic

This comment has been minimized.

Copy link
Member

Xenopathic commented Jan 9, 2015

In addition, this change would be nice to have in oC 8. More performance and new unit tests!

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Jan 9, 2015

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

@owncloud-bot

This comment has been minimized.

Copy link
Contributor

owncloud-bot commented Jan 10, 2015

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

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 10, 2015

👍 this is even recommended in the official PHP manual http://php.net/manual/en/function.strstr.php (second note)

@MorrisJobke

This comment has been minimized.

Copy link
Member

MorrisJobke commented Jan 10, 2015

@karlitschek @DeepDiver1975 Please approve this for 8.0

@karlitschek

This comment has been minimized.

Copy link
Member

karlitschek commented Jan 10, 2015

yes. makes sense 👍

MorrisJobke added a commit that referenced this pull request Jan 10, 2015

Merge pull request #13224 from owncloud/simplify-is-valid-path-and-ad…
…d-unit-tests

Simplify isValidPath and add unit tests

@MorrisJobke MorrisJobke merged commit c91d47e into master Jan 10, 2015

1 check passed

default Merged build finished.
Details

@MorrisJobke MorrisJobke deleted the simplify-is-valid-path-and-add-unit-tests branch Jan 10, 2015

@MorrisJobke MorrisJobke added this to the 8.0-current milestone Jan 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment