Proposed removal of some static checks for phtml template files #160

nathanjosiah · 2019-05-10T21:33:01Z

No description provided.

magento-cicd2 · 2019-05-10T21:33:05Z

All committers have signed the CLA.

nathanjosiah · 2019-05-10T21:35:05Z

These changes were already approved by @buskamuza and @piotrekkaminski in an internal meeting. PR opened for further discussion

melnikovi · 2019-05-10T21:40:40Z

design-documents/static-checks-for-templates.md

+|-----|---------|
+| `Magento2.Files.LineLength` |HTML is frequently much cleaner when kept on single lines even if they are longer. This sniff also incorrectly counts lines containing PHP.|
+| <ul><li>`Squiz.ControlStructures.ControlSignature.NewlineAfterOpenBrace`</li><li> `Squiz.WhiteSpace.ScopeClosingBrace`</li><li>`PEAR.ControlStructures.ControlSignature`</li></ul>|Inline PHP can be cleaner and easier to read in many cases within a PHTML document. <br/><br/>  Additionally, the `PEAR.ControlStructures.ControlSignature.Found` sniff disallows any inline control structures but allows the alternative control syntax (if (foo): blah endif; ) as well as ternaries (foo ? bar : baz) which are the same thing. <br/> <br/> These sniffs also incorrectly detect the control signatures leading to a mixed set of cases where it is actually flagged. It seems to only flag some of these when the endif or } is on the same line as the if|
+| `Squiz.Operators.IncrementDecrementUsage`|Very common in templating|


What does this one disallow? $i++, $i--?

@melnikovi Yes, as well as += and -=

Understood, agree that we need this in templates.

Very common in any loop. Simply remove this rule completely.

melnikovi · 2019-05-10T21:44:30Z

design-documents/static-checks-for-templates.md

+
+|Sniff|Rationale|
+|-----|---------|
+| `Magento2.Files.LineLength` |HTML is frequently much cleaner when kept on single lines even if they are longer. This sniff also incorrectly counts lines containing PHP.|


Could you please explain what does Squiz.WhiteSpace.ScopeClosingBrace check? Do you propose completely remove PEAR.ControlStructures.ControlSignature? I think alternative syntax is more readable.

It works fine for regular php files where it asserts that control structures look like this:

if ($foo) { //something }

However for phtml files it gives different results based on the context but essentially something like this:

<foo <?php if ($foo) { ?>checked<?php } ?> <foo <?php if ($foo) : ?>checked<?php endif; ?>

is required to be turned into

<foo <?php if ($foo) { ?>checked<?php } ?> <foo <?php if ($foo) : ?>checked<?php endif; ?>

or some other weird format.

phpcbf automatically convert it to this at the moment.

@michaelyu0123 Yes I understand but the purpose was for code to be cleaner, this is messy.

So PEAR.ControlStructures.ControlSignature requires to add new line? Then I'm ok with removing it. Does Squiz.WhiteSpace.ScopeClosingBrace check that whitespace exist or doesn't exist before closing tag?

I think that this syntax
<?php if ($foo): ?><h2>Hello world</h2><?php endif ?>
or

<?php if ($foo): ?> <h2>Hello world</h2> <?php endif ?>

both are good examples. Do we preserve this style?

@melnikovi

So PEAR.ControlStructures.ControlSignature requires to add new line?

Yes, and only for some cases so it's inconsistent.

Does Squiz.WhiteSpace.ScopeClosingBrace check that whitespace exist...

There are different checks for proper (non-newline) whitespace.

Do we preserve this style?

Yes, that's the idea for the PR is that I think we should preserve these styles

melnikovi · 2019-05-10T21:46:38Z

design-documents/static-checks-for-templates.md

+
+### Design
+
+To address this issue for the immediate efforts as well as for all developers in the future I am proposing to remove the following sniffs


Does this mean that you also propose to start removing @codingStandardsIgnore which would require fixing existing errors?

Yes, and all files affected by the security efforts (hundreds of phtml files) will be cleaned up as part of our initial merges in the next couple weeks.

I may be wrong, but I believe there is already a backlog item to remove all @codingStandardsIgnore annotations.

orlangur

Please don't touch any sniffs which belong to PSR-2. Such code needs to be adopted.

Among all described sniffs Magento2.Security.LanguageConstruct.DirectOutput was implemented wrongly in the first and all other seem to be adoptable.

orlangur · 2019-05-11T09:49:13Z

design-documents/static-checks-for-templates.md

+### Overview
+
+At the time of this writing only 85% of the `*.phtml` files in Magento core are being statically 
+tested due to `@codingStandardsIgnore` present in over 700 templates. There is a current security initiative 


Looks like you meant @codingStandardsIgnoreFile here.

orlangur · 2019-05-11T09:49:28Z

design-documents/static-checks-for-templates.md

+
+At the time of this writing only 85% of the `*.phtml` files in Magento core are being statically 
+tested due to `@codingStandardsIgnore` present in over 700 templates. There is a current security initiative 
+that requires a large number of templates to be modified and they need to be covered by static checks moving 


Link to initiative, please?

https://jira.corp.magento.com/browse/MC-15343

I don't think maintainers have access to JIRA after acquisition by Adobe anymore.

Is it too sensitive for public discussion or at least in some private Slack channel?

It's sort of sensitive. The changes will go to 2.3-develop. The changes essentially involve cleaning certain things up in every module.

Ok. I believe it would be enough to share what kind of changes to code and tests is supposed to be done so that it can be aligned with other coding standard compliance activities.

In scope of this task the changes are described in this proposal - cleanup templates (by removing phpcs suppression).

orlangur · 2019-05-11T09:50:06Z

design-documents/static-checks-for-templates.md

+forward as part of the Acceptance Criteria of the initiate. 
+
+However, due to the lack of current static template checks the codebase has thousands of static template errors and will require many 
+hours to address every issue in every file. Additionally, many of the tests are not reasonable, checking incorrectly, or simply gratuitous for `*.phtml` templates. 


This is not true as it does not require a lot of manual work.

Unfortunately this is not true. We have about five teams that have attempted to adopt the principles and it is tripling the amount of time needed to complete these tasks.

LOL :) I did it once for 2.2-develop before in magento/magento2#9367 already. That sounds like a good challenge to me, want to get all the details and solve the problem then.

Looks like a big effort thank you for that contribution. Unfortunately we have other more important business priorities than cleaning up every file and potentially breaking things. Plus, the purpose of this request is to lower the bar for entry and make it easier write and change templates without massive efforts such as the one you linked to.

orlangur · 2019-05-11T10:03:59Z

design-documents/static-checks-for-templates.md

+
+|Sniff|Rationale|
+|-----|---------|
+| `Magento2.Files.LineLength` |HTML is frequently much cleaner when kept on single lines even if they are longer. This sniff also incorrectly counts lines containing PHP.|


It may mean that there are too much nesting levels. Such violations may be fixed with automated tooling.

Needs to be provided some insight on current situation to decide.

We have no standards for HTML nesting level requirements that we in any way claim to follow. Therefore the comment about "too much nesting levels" seems like a personal opinion rather than a requirement. Please share with me any requirement if there is one documented. All of the automated tooling works for PHP or HTML but I've never seen anything that works for both. Additionally, our existing tooling only works for PHP.

That is not what I was saying :) I didn't say a word regarding nesting level enforcement.

Line length is a good limitation to start from as high nesting level by implication leads to longer lines

I did some experiments before with automated code formatting by setting absolute line limit to 80 or 120 characters and it worked smoothly

Here's a very common example of cases where line length is arbitrary and would be messy to try and break it apart:

<label class="admin__field-label" for="sidebar-<?= $block->escapeHtmlAttr($block->getSidebarStorageAction()) ?>-<?= $block->escapeHtmlAttr($block->getIdentifierId($_item)) ?>"> </label> </td> </tr> </tbody> </table> </div> </div> </div>

I do share the sentiment that every error could be addressed in some way however it doesn't make too much sense to do it. High levels of nesting in PHP are a code smell and a sign that it can be refactored to have less code paths and run more efficiently. Long lines have a similar idea. However, with HTML nesting is a natural part of the language and at most there can be a mild correlation between nesting level and good design. So, forcing arbitrary constraints on PHP because of HTML structure is where I disagree with this approach.

Also, to reiterate what I've said elsewhere, the purpose of this PR is to make templates easier to write and refactor without risking too much change. I understand that in some cases this may come at cost of slightly longer lines but engineering is all about finding reasonable compromises and solutions.

orlangur · 2019-05-11T10:04:51Z

design-documents/static-checks-for-templates.md

+|-----|---------|
+| `Magento2.Files.LineLength` |HTML is frequently much cleaner when kept on single lines even if they are longer. This sniff also incorrectly counts lines containing PHP.|
+| <ul><li>`Squiz.ControlStructures.ControlSignature.NewlineAfterOpenBrace`</li><li> `Squiz.WhiteSpace.ScopeClosingBrace`</li><li>`PEAR.ControlStructures.ControlSignature`</li></ul>|Inline PHP can be cleaner and easier to read in many cases within a PHTML document. <br/><br/>  Additionally, the `PEAR.ControlStructures.ControlSignature.Found` sniff disallows any inline control structures but allows the alternative control syntax (if (foo): blah endif; ) as well as ternaries (foo ? bar : baz) which are the same thing. <br/> <br/> These sniffs also incorrectly detect the control signatures leading to a mixed set of cases where it is actually flagged. It seems to only flag some of these when the endif or } is on the same line as the if|
+| `Squiz.Operators.IncrementDecrementUsage`|Very common in templating|


Very common in any loop. Simply remove this rule completely.

orlangur · 2019-05-11T10:05:55Z

design-documents/static-checks-for-templates.md

+| `Magento2.Files.LineLength` |HTML is frequently much cleaner when kept on single lines even if they are longer. This sniff also incorrectly counts lines containing PHP.|
+| <ul><li>`Squiz.ControlStructures.ControlSignature.NewlineAfterOpenBrace`</li><li> `Squiz.WhiteSpace.ScopeClosingBrace`</li><li>`PEAR.ControlStructures.ControlSignature`</li></ul>|Inline PHP can be cleaner and easier to read in many cases within a PHTML document. <br/><br/>  Additionally, the `PEAR.ControlStructures.ControlSignature.Found` sniff disallows any inline control structures but allows the alternative control syntax (if (foo): blah endif; ) as well as ternaries (foo ? bar : baz) which are the same thing. <br/> <br/> These sniffs also incorrectly detect the control signatures leading to a mixed set of cases where it is actually flagged. It seems to only flag some of these when the endif or } is on the same line as the if|
+| `Squiz.Operators.IncrementDecrementUsage`|Very common in templating|
+| `Magento2.Security.LanguageConstruct.DirectOutput`|This makes sense for PHP files but not for templates where the whole purpose is to output content.|


The sniff must specify that it is only applicable to php then.

That is the whole purpose of this PR, these removals only affect PHTML files.

I mean there is no reason to do any changes to standard, just specify in Magento2.Security.LanguageConstruct.DirectOutput sniff that it do not touch phtml.

Can you elaborate? I think this is what is recommended - do not run the rule for phtml templates. You want to put this logic in the rule itself instead of limiting it in the ruleset? If yes, why?

orlangur · 2019-05-11T10:10:45Z

design-documents/static-checks-for-templates.md

+#### Acceptance Criteria Fulfillment
+
+1. Acceptance Criteria 1
+ The proposed sniffs exclude `*phtml` files preferably in a way that doesn't affect the `magento-coding-standards` repo.


This totally does not make sense. For the sake of "unification" MEQP standard was renamed to "Magento2 Coding Standard" without a proper adoption of core codebase.

Now because this "new" coding standard was placed in a separate repo (for no reason, as you all probably remember) we are trying to not "affect" it.

This may not actually be possible due to how our CICD tooling works. It is only a desired goal.

I'm saying that such goal sounds absurd. We should have only one comprehensive coding standard for all situations and adjust it properly until this is achieved.

I agree. However, because of how our CICD tooling validates builds I'm not sure this will be feasible at the moment.

nathanjosiah · 2019-05-11T13:04:22Z

@orlangur All of these sniff removals are only going to affect PHTML templates. Since PSR2 only applies to PHP files (that is to say, they define no rules surrounding PHP formatting in HTML templates) we could theoretically disable all PHTML sniffs for PSR2 and still be in compliance.

orlangur · 2019-05-11T13:39:21Z

@nathanjosiah,

Since PSR2 only applies to PHP files

This is a huge misunderstanding. PSR2 is fully applicable to PHTML files (it was not possible to use it before until PHPCS 1.x contained some bugs but now totally possible).

melnikovi · 2019-05-15T15:08:14Z

@orlangur I'm going to accept this PR as it's approved by two people. If you have any suggestions please submit PR that alters this proposal.

orlangur · 2019-05-16T06:41:08Z

@melnikovi ROFL 🤣 Hope someday I'll get used to this game.

Proposed removal of some static checks for phtml template files

c247499

nathanjosiah mentioned this pull request May 10, 2019

Discussion of static test rule removals for phtml files. #159

Closed

melnikovi reviewed May 10, 2019

View reviewed changes

melnikovi self-assigned this May 10, 2019

orlangur suggested changes May 11, 2019

View reviewed changes

buskamuza approved these changes May 14, 2019

View reviewed changes

melnikovi approved these changes May 14, 2019

View reviewed changes

melnikovi merged commit 0811f81 into magento:master May 15, 2019

nathanjosiah deleted the static-checks-for-templates branch May 16, 2019 12:56

lenaorobei mentioned this pull request Jun 5, 2019

Added excludes for phtml files magento/magento-coding-standard#112

Merged


		### Design

		To address this issue for the immediate efforts as well as for all developers in the future I am proposing to remove the following sniffs

Proposed removal of some static checks for phtml template files #160

Proposed removal of some static checks for phtml template files #160

Conversation

nathanjosiah commented May 10, 2019

magento-cicd2 commented May 10, 2019 • edited Loading

nathanjosiah commented May 10, 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

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

orlangur left a 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

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

nathanjosiah May 14, 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

buskamuza May 14, 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

nathanjosiah commented May 11, 2019

orlangur commented May 11, 2019

melnikovi commented May 15, 2019

orlangur commented May 16, 2019

magento-cicd2 commented May 10, 2019 •

edited

Loading

nathanjosiah May 14, 2019 •

edited

Loading

buskamuza May 14, 2019 •

edited

Loading