Assign severity to each sniff #37

lenaorobei · 2019-02-11T21:51:12Z

changed rules severities based on [Proposal] Rules Severities for Marketplace Technical Review #14

- changed rules severities based on #14

magento-cicd2 · 2019-02-11T21:51:14Z

All committers have signed the CLA.

- fix code style

orlangur · 2019-02-13T16:15:35Z

Magento/Sniffs/PHP/DiscouragedFunctionSniff.php

@@ -235,7 +235,7 @@ class DiscouragedFunctionSniff extends ForbiddenFunctionsSniff
     *
     * @return void
     */
-    protected function addError($phpcsFile, $stackPtr, $function, $pattern = null)
+    protected function addError($phpcsFile, $stackPtr, $function, $pattern=null)


Where such spaces removal around = comes from?

You're right. I missed that part of PSR2 standard. Will fix.

I fixed some rules to be compatible with PSR2.
Anyway, severities are all about Marketplace checks so you don't have to worry about Magento core. For core all findings will lead to build failure.

@lenaorobei I wouldn't worry about Magento core is there was a separate Marketplace coding standard based on core one. What is the reason to mix such totally different concerns?

@orlangur why do you think so? Extension developers should follow the same rules as core developers.

orlangur · 2019-02-13T16:17:04Z

Magento/ruleset.xml

        <type>warning</type>
    </rule>
-    <rule ref="Squiz.PHP.NonExecutableCode">
+    <rule ref="PSR2.Methods.FunctionCallSignature">


Revising PSR rules severities is not a good idea at all.

Will review this. Thanks for pointing.

orlangur · 2019-02-13T18:00:39Z

Hi @lenaorobei, not sure if it's related to this PR. I always wanted to have Magento codebase following PSR-2 as strictly as possible. Some warnings, like leading underscores, are really hard to fix all over the codebase.

Do we have a separate ruleset for new files planned? @paliarush mentioned something like this. In such ruleset we would, for example, convert some warnings to errors just to have new code even cleaner in terms of PSR-2.

lenaorobei · 2019-02-13T21:07:05Z

@orlangur

I always wanted to have Magento codebase following PSR-2 as strictly as possible.

This is definitely what we want to archive and what we are aiming to. The purpose of this coding standard is validation not only Magento codebase but also developers extensions on Marketplace, so we work iteratively in that regard.

I reviewed ruleset and made it follow PSR2. I understand that severities do not make sense for core, but we need them for Marketplace technical review. We even have a scoring approach proposal, which should improve the overall code quality of Marketplace extensions.

- review fixes; - made it follow PSR2;

orlangur · 2019-02-15T11:28:45Z

@lenaorobei,

severities do not make sense for core, but we need them for Marketplace technical review

It means that it should be a separate standard and there is no need to spoil core coding standard :)

Of course, we shouldn't have two unrelated standards, like it was with MEQP and core standard, but the standard for Marketplace must be based on core one. Some new rules may be introduced in it which are not applicable to core, some severities adjusted to fulfill needs of Marketplace extension review.

But we cannot fulfill contradicting requirements in the scope of a single standard.

melnikovi · 2019-02-15T15:11:04Z

Magento/ruleset.xml

+        <severity>9</severity>
+        <type>warning</type>
+    </rule>
+    <rule ref="Magento.Security.XssTemplate">


I propose we increase severity here, we should be very strict about this.

Unfortunately we cannot restrict this rule right now since its possible issue. We will work iteratively in that regard.

melnikovi · 2019-02-15T16:11:13Z

Magento/ruleset.xml

    </rule>
-    <rule ref="Magento.Translation.ConstantUsage">
+    <rule ref="Generic.Files.ByteOrderMark">


Propose increases severity to 9. We can rename section to "Possible security and issues that may cause bugs".

melnikovi · 2019-02-15T16:11:38Z

Magento/ruleset.xml

    </rule>
-    <rule ref="Magento.Strings.ExecutableRegEx">
-        <severity>10</severity>
+    <rule ref="Squiz.PHP.NonExecutableCode">


Propose to increase severity to 9.

melnikovi · 2019-02-15T16:12:41Z

Magento/ruleset.xml

+        <severity>7</severity>
+        <type>warning</type>
+    </rule>
+    <rule ref="Magento.PHP.DiscouragedFunction">


Per our discussion, let's increase severity to 9 because it checks for functions that may lead to security issues.

melnikovi · 2019-02-15T16:13:20Z

Magento/ruleset.xml

-    <rule ref="Magento.NamingConvention.ReservedWords">
-        <severity>10</severity>
+        <severity>8</severity>
+        <type>warning</type>
    </rule>
    <rule ref="Magento.PHP.DateTime">


Propose increase severity to 9 as it may cause bugs.

melnikovi · 2019-02-15T16:13:49Z

Magento/ruleset.xml

+    </rule>
+
+    <!-- Severity 9 warnings: Possible security issues. -->
+    <rule ref="Magento.Security.IncludeFile">


Propose increase severity to 9 as it may cause security issues.

I assume you mean "increase to 10". Done.

- review fixes

lenaorobei · 2019-02-15T17:54:59Z

@melnikovi changed to proposed severities.

Updated WIKI page as well.

paliarush · 2019-02-19T14:54:11Z

Magento/ruleset.xml

+        <type>error</type>
+    </rule>
+
+    <!-- Severity 9 warnings: Possible security and issues that may cause bugs. -->


How will this work with PhpStorm? I thought we are going to split the rules with different severity into different rulesets

PHPStorm will show all issues regardless of severities. I didn't get what you mean here.

split the rules with different severity into different rulesets

Severities are for Marketplace technical review only. What is the purpose of splitting into different rulesets here?

Do we expect to see all issues with any severity highlighted by PhpStorm? Also, this ruleset will be used for Magento core, right?

Yes, we expect to see all issues.
Yes, this ruleset will be used for Magento core.

I believe we agreed to move out potentially false-positive rules to separate ruleset to be used with incremental checks.

Right now all checks are used with incremental approach. The same behaviour will preserved for the first release. We can discuss false-positive rulesets for future improvements.

- review fixes

paliarush · 2019-02-28T15:44:40Z

Magento/ruleset.xml

@@ -305,6 +306,7 @@
    <rule ref="PSR2.Classes.PropertyDeclaration">
        <severity>6</severity>
        <type>warning</type>
+        <exclude name="PSR2.Classes.PropertyDeclaration.Underscore"/>


Why are these excluded? We do not want any variables and method with underscore prefixes.

It's because a lot of legacy code in Magento. When extension developers extend some classes they need to suppress such findings.

We need some strategy for the new code to enable such rules there.

Not in the roadmap

…ck (#37) https://jira.corp.magento.com/browse/AC-949

REPO-70: [EQP][Sniffs Consolidation] Assign severity to each sniff

a5a05a6

- changed rules severities based on #14

lenaorobei requested a review from paliarush February 11, 2019 21:51

lenaorobei changed the title ~~REPO-70: [EQP][Sniffs Consolidation] Assign severity to each sniff~~ Assign severity to each sniff Feb 11, 2019

REPO-70: [EQP][Sniffs Consolidation] Assign severity to each sniff

f9f75a3

- fix code style

orlangur previously requested changes Feb 13, 2019

View reviewed changes

REPO-70: [EQP][Sniffs Consolidation] Assign severity to each sniff

cdf9901

- review fixes; - made it follow PSR2;

lenaorobei requested a review from melnikovi February 15, 2019 14:27

melnikovi reviewed Feb 15, 2019

View reviewed changes

REPO-70: [EQP][Sniffs Consolidation] Assign severity to each sniff

9f42f25

- review fixes

melnikovi previously approved these changes Feb 15, 2019

View reviewed changes

paliarush reviewed Feb 19, 2019

View reviewed changes

paliarush previously approved these changes Feb 27, 2019

View reviewed changes

REPO-70: [EQP][Sniffs Consolidation] Assign severity to each sniff

9af88d2

- review fixes

lenaorobei dismissed stale reviews from paliarush and melnikovi via 9af88d2 February 28, 2019 15:30

REPO-70: [EQP][Sniffs Consolidation] Assign severity to each sniff

1d0c79a

- review fixes

lenaorobei requested a review from paliarush February 28, 2019 15:31

paliarush reviewed Feb 28, 2019

View reviewed changes

paliarush approved these changes Feb 28, 2019

View reviewed changes

lenaorobei merged commit 9706441 into develop Feb 28, 2019

lenaorobei mentioned this pull request Feb 28, 2019

[Proposal] Rules Severities for Marketplace Technical Review #14

Closed

lenaorobei deleted the REPO-70-Assign-severity-to-each-sniff branch February 28, 2019 20:21

magento-devops-reposync-svc pushed a commit that referenced this pull request Aug 31, 2021

AC-949: Create unit test for Magento2\Less\PropertiesSortingSniff che…

faddd30

…ck (#37) https://jira.corp.magento.com/browse/AC-949

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assign severity to each sniff #37

Assign severity to each sniff #37

lenaorobei commented Feb 11, 2019

magento-cicd2 commented Feb 11, 2019 •

edited

Loading

orlangur Feb 13, 2019

lenaorobei Feb 13, 2019

lenaorobei Feb 13, 2019

orlangur Feb 25, 2019

lenaorobei Feb 28, 2019

orlangur Feb 13, 2019

lenaorobei Feb 13, 2019 •

edited

Loading

orlangur commented Feb 13, 2019

lenaorobei commented Feb 13, 2019

orlangur commented Feb 15, 2019

melnikovi Feb 15, 2019

lenaorobei Feb 15, 2019

melnikovi Feb 15, 2019 •

edited

Loading

melnikovi Feb 15, 2019

melnikovi Feb 15, 2019

melnikovi Feb 15, 2019

melnikovi Feb 15, 2019

lenaorobei Feb 15, 2019

lenaorobei commented Feb 15, 2019

paliarush Feb 19, 2019

lenaorobei Feb 19, 2019

paliarush Feb 19, 2019

lenaorobei Feb 19, 2019

paliarush Feb 19, 2019

lenaorobei Feb 19, 2019

paliarush Feb 28, 2019

lenaorobei Feb 28, 2019

paliarush Feb 28, 2019

Assign severity to each sniff #37

Assign severity to each sniff #37

Conversation

lenaorobei commented Feb 11, 2019

magento-cicd2 commented Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenaorobei Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

orlangur commented Feb 13, 2019

lenaorobei commented Feb 13, 2019

orlangur commented Feb 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melnikovi Feb 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lenaorobei commented Feb 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magento-cicd2 commented Feb 11, 2019 •

edited

Loading

lenaorobei Feb 13, 2019 •

edited

Loading

melnikovi Feb 15, 2019 •

edited

Loading