If a markdown formatted link exceeds 80 characters, PHPCS throws a LineLengthSniff warning. Is this intended? If so, is there documentation on how to format a markdown README.md file to avoid this warning?

Comments

markdorison created an issue. See original summary.

jonathan1055’s picture

Yes, I also have this problem, in the Rules module readme.md. I have used --ignore=*.md so that the file is not tested (this is for Travis builds where we need a 0 exit code from phpcs)

If the link cannot be shortened then we could alter the sniff not to report this "error".

ApacheEx’s picture

jonathan1055’s picture

Hi ApacheEx,

Thanks for the link, I seem to recall I had also noticed that the extensions list is not adhered to. But the actual problem I am reporting is how to get round the fact that markdown links currently cannot be wrapped to a new line so they often extend past 80 and cause a standards failure. The .md file can be checked for other standards so excluding the file entirely is not what we want.

Jonathan

Dropa’s picture

Coder in fact should be little less forcing on the 80 character rule with markdown, there are two examples where it is not easy, if even possible to shrink lines.

There is this line which exceeds 80 mark line because [of this link]

[of this link]: www.example.com/instead-of-this-unnecessarily-long-link-that-itself-could-exceed-80-characters

or say you would like to document usage of some plugin

This is how you would declare interface for entity
```php
/**
 * Provides an interface defining a node entity.
 */
interface NodeInterface extends ContentEntityInterface, EntityChangedInterface, EntityOwnerInterface, RevisionLogInterface, EntityPublishedInterface {
}
```

I would suggest coder to completely ignore line lengths in codeblocks and in links (at least those that has been described in own line)

jonathan1055’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev

Yes the sniff probably does need some work. In the meantime, if you have a particular file in your module that fails on linelength you can do like Rules does, and add the following to the project's phpcs.xml.dist file

  <rule ref="Drupal.Files.TxtFileLineLength.TooLong">
    <!-- Do not run this sniff on README.md. -->
    <exclude-pattern>README.md</exclude-pattern>
  </rule>
pfrenssen’s picture

This also happens when referring to internal links by the way so the standard detection that we are doing right now (check if the line includes "https") is not sufficient. Internal links are quite common in Markdown files so you can link documentation pages internally and have it published on different domains like drupal.org and github.

I think a good solution could be to do something similar as is done in markdownlint. Their line length rule will check if there are no spaces occurring after the 80 character limit.

We could do a check like: if the line extends beyond 80 characters and does not contain any whitespace after character #40 then we will not throw the warning.

Ref. https://github.com/markdownlint/markdownlint/blob/master/docs/RULES.md#m...

pfrenssen’s picture

@Dropa it is a good suggestion to ignore line lengths in code blocks but I think it's best to handle that in a separate issue.

itamair’s picture

Priority: Normal » Critical

this is heavily affecting (in a really un-smart way) the Security Process Review of the D* Geofield Map project: https://www.drupal.org/project/projectapplications/issues/3071886#commen...

Please indicate a feasible way to bypass this impediment.

jonathan1055’s picture

Hi itamair,
Have you tried the solution I suggested in #6 above ?

itamair’s picture

thanks @jonathan1055 ... but your workaround seem not to work with the Pareview.sh process (and indeed didn't work for my pareview) as more clearly explained time ago:
https://www.drupal.org/project/pareviewsh/issues/2865650

pfrenssen’s picture

Status: Active » Needs review

Here is a proposed fix: https://github.com/pfrenssen/coder/pull/57

@itamair can you check if this solves the false positive for you?

itamair’s picture

thanks @pfrenssen ... and sorry,
but since I applied the #6 workaround (and then also removed it) the phpcs coding standard is not reporting those Readme.md link "longer than 80 chars" warnings anymore.
I exactly don't know why ... but actually I cannot test your PR,
because I cannot recreate this, anymore.

I should also mention that upgraded to Drupal Coder 8.x-3.x (I was using 8.x-2.x) before

(somehow, thanks to a smart and more practical reviewer, the Geofield Map module finally passed the Security Policy Review: https://www.drupal.org/project/projectapplications/issues/3071886)

I hope it would be easier for you to reproduce the Readme.md link "longer than 80 chars" warning (in a test module) and check your PR fix ...

pfrenssen’s picture

No problem, I'm glad you managed to solve it!

I have a test in the PR using two types of links that were previously failing. These are now fixed. I was just curious if your case was also solved.

  • klausi committed c31df23 on 8.x-3.x authored by pfrenssen
    fix(TxtFileLineLength): Long Markdown links may exceed line length...
klausi’s picture

Priority: Critical » Normal
Status: Needs review » Fixed

Thanks, that makes sense. Merged!

I don't think this is a critical issue since people can always silence this sniff in their config. Coding standard issues are also not application blockers in the security advisory coverage queue.

itamair’s picture

yes @klausi ... I agree, and that should be the case, in both theory and practice.
BUT it seems that some (not so skilled) reviewers make your Security Advisory Review rely mostly on the automatic Pareview.sh outcome,
pretending you to fix all the single (even the less meaningful) warnings (and not errors) before moving forward with the real security review itself.
Upon my direct experience that is a critical issue that makes the Security Advisory Review process so bureaucratic and cumbersome (not to say a real pain in the xxx ...).
Happy I got rid of it ... and just concerned for new developers.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.