Skip to content

fix Notice: Undefined index: 0777 in Stat->mergeMeta #19

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ms-slasher13
Copy link

fix undefined index in mergeMeta
Notice: Undefined index: 0777 in Twistor\Flysystem\Plugin\Stat->mergeMeta() (line 157 of \vendor\twistor\flysystem-stream-wrapper\src\Flysystem\Plugin\Stat.php)

using flysystem 1.0.63 and "Local" adapter

see #12
see #15

…ergeMeta() (line 157 of \vendor\twistor\flysystem-stream-wrapper\src\Flysystem\Plugin\Stat.php).
@ms-slasher13 ms-slasher13 changed the title fix Notice: Undefined index: 0777 in Twistor\Flysystem\Plugin\Stat->m… fix Notice: Undefined index: 0777 in Stat->mergeMeta Jan 9, 2020
@Lewiscowles1986
Copy link

The appveyor failed on chocolatey windows package manager. This looks like it should fix master

@sylvainar
Copy link

Hey,
Currently struggling with this, will this patch be applied?

@hexus
Copy link

hexus commented Sep 14, 2020

@twistor I too would love to see this or a similar fix merged and released, as it should fix PHP 7.3/7.4 compatibility.

@drewek-smf
Copy link

+1

@snapshotpl
Copy link
Contributor

@twistor ping

Copy link

@benjifisher benjifisher left a comment

Choose a reason for hiding this comment

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

Thanks for this improvement! This change fixes the problem for my current project.

I made some suggestions: one little tweak to the code and several changes to be consistent with the commenting standards used in this project.

/**
* Normalize a permissions string.
*
* @param string $permissions

Choose a reason for hiding this comment

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

Suggested change
* @param string $permissions
* @param string $permissions A permissions string, such as '644' or
* 'drw-rw-r--'

*
* @param string $permissions
*
* @return int

Choose a reason for hiding this comment

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

Suggested change
* @return int
* @return int The numeric version of the permissions.

return $permissions & 0777;
}

// remove the type identifier

Choose a reason for hiding this comment

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

Suggested change
// remove the type identifier
// Remove the type identifier.

// remove the type identifier
$permissions = substr($permissions, 1);

// map the string rights to the numeric counterparts

Choose a reason for hiding this comment

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

Suggested change
// map the string rights to the numeric counterparts
// Map the string rights to the numeric counterparts.

$map = ['-' => '0', 'r' => '4', 'w' => '2', 'x' => '1'];
$permissions = strtr($permissions, $map);

// split up the permission groups

Choose a reason for hiding this comment

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

Suggested change
// split up the permission groups
// Split up the permission groups.

// split up the permission groups
$parts = str_split($permissions, 3);

// convert the groups

Choose a reason for hiding this comment

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

Suggested change
// convert the groups
// Convert the groups.

return array_sum(str_split($part));
};

// converts to decimal number

Choose a reason for hiding this comment

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

Suggested change
// converts to decimal number
// Converts to decimal number.

}

// remove the type identifier
$permissions = substr($permissions, 1);

Choose a reason for hiding this comment

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

Suggested change
$permissions = substr($permissions, 1);
$permissions = substr($permissions, -9);

This will work with or without the initial d or - for regular file vs. directory. I think this also makes it clear that we expect 9 characters.

I considered padding with - characters to ensure 9 characters, but I am not sure when that would be likely to make a difference. Pad on the left or the right? Maybe it is a bad idea to fix problems that no one has.

Copy link

@hexus hexus Feb 11, 2021

Choose a reason for hiding this comment

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

I feel like the original code is closer to what the comment says it's trying to do, but indeed -9 is clearer for the actual intent here: extracting the permission groups in isolation.

Padding would make it mildly safer to use with the code below it, and if you're using substr from the right then I'd say it makes sense to pad the left, if at all.

Suggested change
$permissions = substr($permissions, 1);
$permissions = substr(str_pad($permissions, 9, '-', STR_PAD_LEFT), -9);

The comment could be updated to:

// Extract the permission groups.

The trickiest thing is how vague the file metadata is with Flysystem, but I suppose it has to be (for v1.x at least 😉).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants