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

Add new rule to add visibility for only method and property inside a … #71

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

PoulainMaxime
Copy link

@PoulainMaxime PoulainMaxime commented Oct 7, 2022

…class

Questions Answers
Description? Adding new rule for set visibility only for method and properties. We disabled this rule only for 'const' case
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Remove keyword public for method or property and run csfixer, he should add the missing keyword
Possible impacts? none

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Hello @PoulainMaxime that is a very good idea

but this means that if we merge this, any module using php-dev-tools will see their CI go red if they have not prefixed every const with a clear scope, right?

@Quetzacoalt91
Copy link
Member

Hi @matks,

When you disable the rule for a specific case, it does not check it anymore. This means that whatever you have (with visibility or without it), it won't report anything.
=> No CI will go red with this update of rule if they already applied it on consts before.

@matks
Copy link
Contributor

matks commented Oct 11, 2022

Hi @matks,

When you disable the rule for a specific case, it does not check it anymore. This means that whatever you have (with visibility or without it), it won't report anything. => No CI will go red with this update of rule if they already applied it on consts before.

Then I approve it 😄

@PoulainMaxime
Copy link
Author

Hi @matks,
When you disable the rule for a specific case, it does not check it anymore. This means that whatever you have (with visibility or without it), it won't report anything. => No CI will go red with this update of rule if they already applied it on consts before.

Then I approve it 😄

Hi @matks, if everything is good, is it possible to merge it? Thanks !

@atomiix
Copy link

atomiix commented Oct 13, 2022

The PR seems good but, why? 😅 Can you explain why you're disabling the rule to force to set the visibility for const?

@matks
Copy link
Contributor

matks commented Oct 13, 2022

Can you explain why you're disabling the rule to force to set the visibility for const?

I guess it's because a lot of php developers do not prefix consts by visibility 😅

@Quetzacoalt91
Copy link
Member

We actually have another reason than habits. :)

PS Facebook is reported to fail of early versions of PS 1.7 (reported by @eternoendless earlier this month). This was PHP-Parser (with PREFER_PHP7 flag) in the core unable to read the code after this rule being applied.
We made the change to improve the compatibility of all modules with PrestaShop 1.7.x.

@Quetzacoalt91
Copy link
Member

Sorry for being pushy, but is there anything I can do to go on the review?
When released, the change would be made on the validator as well. At the moment it still asks the public to be added in front of constants, and I'd like to avoid that if it breaks early versions on PrestaShop 1.7.

Thanks

@matks
Copy link
Contributor

matks commented Oct 18, 2022

PR looks good to me 👍 @atomiix do you have some last worries or is it OK?

@matks
Copy link
Contributor

matks commented Oct 18, 2022

Thank you @atomiix @PoulainMaxime and @Quetzacoalt91

@matks matks merged commit 843275b into PrestaShop:master Oct 18, 2022
@matks matks added this to the 4.2.2 milestone Oct 18, 2022
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.

5 participants