Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Dec 29, 2017

If you have a file in your root folder. And you rename that file. We
will try to find who has access to the parent. In this case the parent
of is the root folder of the user. So sections is only 3 elements big.

Found on our sentry instance.

If you have a file in your root folder. And you rename that file. We
will try to find who has access to the parent. In this case the parent
of is the root folder of the user. So sections is only 3 elements big.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
$accessList['ownerPath'] = '/' . $sections[3];

$accessList['ownerPath'] = '/';
if (count($sections) === 4) {
Copy link
Member

Choose a reason for hiding this comment

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

I would use an isset($sections[3]) because it makes semantically more sense (4+ elements is fine and it actually tests, the element that is actually used later - [0 => 'a', 1 => 'b', 2 => 'c', 4 => 'd'] is also an array of the length 4 😉 (even if it is not typical to be the case here)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

but sure

Copy link
Member

Choose a reason for hiding this comment

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

I know, this is just about better practises and easier to understand code snippets.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, but yeah isset on 3 seems more logical.

Thanks for finding the issue.

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the allow_root_file_rename branch from 54a1855 to 50e2398 Compare January 2, 2018 16:15
@nickvergessen nickvergessen merged commit 1449250 into master Jan 2, 2018
@nickvergessen nickvergessen deleted the allow_root_file_rename branch January 2, 2018 16:16
@nickvergessen
Copy link
Member

@nickvergessen
Copy link
Member

Or is it just a php notice?

@MorrisJobke
Copy link
Member

@nickvergessen Looking at #163 I would backport it to 12.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants