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

Make created cache dir mask as option and default to 0777 #26

Merged
merged 2 commits into from
Jan 13, 2019

Conversation

Vincz
Copy link

@Vincz Vincz commented Dec 26, 2018

Make the created cache dir default to mask 0777 instead of 0775 to avoid permission errors on cache removal. Fix #24

Copy link
Member

@mcg-web mcg-web left a comment

Choose a reason for hiding this comment

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

Thank @Vincz for this contribution! I leave some subjection.

/**
* @var int
*/
protected $cacheDirMask;
Copy link
Member

Choose a reason for hiding this comment

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

Since the value can be set using construct making this private in a first place could ease maintenance.

Suggested change
protected $cacheDirMask;
private $cacheDirMask;

src/Generator/AbstractTypeGenerator.php Outdated Show resolved Hide resolved
@Vincz
Copy link
Author

Vincz commented Dec 27, 2018

The problem with the suggested solution of using the constructor and letting the default to 0755 is that we would need to update also the GraphqlBundle (to add the missing parameter on extends) in order to use the new one.

Or we could use a public static property and do something like:

public static $cacheDirMask = 0755;

And to change it:

Overblog\GraphQLGenerator\Generator\TypeGenerator::$cacheDirMask = 0777;

What do you think @mcg-web ?

@mcg-web
Copy link
Member

mcg-web commented Dec 27, 2018

In GraphQL Bundle modifying this line does the work. Changing dir mask can be consider as BC since this change how files is generated so this will be release in 0.8, the bundle requires 0.7 of this lib so we must modify the bundle anyway. I prefer setting the value of cache dir mask variable only by construct since this is a configuration argument (so no need to change it on compile time). I'm no fan of the global static variable since you loose flexibility between different instances.

Let 0775 as default mask

Co-Authored-By: Vincz <vincent@fullstack.pro>
@Vincz
Copy link
Author

Vincz commented Jan 13, 2019

Hey @mcg-web ! I'll add a PR on the bundle when this one will be merge and a new version set.

@mcg-web mcg-web merged commit 6a131a9 into overblog:master Jan 13, 2019
@mcg-web
Copy link
Member

mcg-web commented Jan 13, 2019

Done @Vincz! The bundle fix should be submit for 0.11 version. Thanks again for your contribution!

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.

None yet

2 participants