Problem/Motivation

The sniff Generic.PHP.DisallowShortOpenTag incorrectly complains when finding <?xml in embeded .yml data structures. This rule could be excluded from all .yml files as it is not needed there.

Steps to reproduce

Use test .yml file containing 

data: "<?xml anything"?>

Here is the actual file that caused the question in the first place, raised in this slack thread

Proposed resolution

Add exclude-pattern to skip .yml files for this sniff

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork coder-3535411

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented

I can work on a MR and add test coverage if this is an agreed way forward.

mandclu’s picture

mandclu commented

I have a project that started throw 15 phpcs errors of the exact type mentioned in the IS and also one other, all in .yml files. I can't personally see what useful feedback we would get from running a php check against .yml files.

jonathan1055 changed the visibility of the branch 3535411-exclude-some-sniffs-on-yml-files to hidden.

jonathan1055’s picture

jonathan1055 commented

Coder is being used to check some useful things in .yml files such as #2841649: Add sniff for _access: 'TRUE' in Drupal 8 routing.yml and probably others. I think most php codesniffer rules will not trigger in .yml files, but some might, for example the one in this issue. Could you link to your pipeline job, or show the file(s) and sniff(s) that are incorrectly flagged. Then they could be excluded here too.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review

I have created https://github.com/pfrenssen/coder/pull/268

I can add test coverage if this is required.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

Thanks, approach makes sense. Left one minor comment on the PR https://github.com/pfrenssen/coder/pull/268

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

Thanks, I have addressed the feedback in the PR. Also added a new test file good/good.yml and tested without the change (to prove coverage) then with the change.
https://github.com/pfrenssen/coder/pull/268/commits

[ignore the change to extentions at the top of the ruleset.yml. I later realised that the recommended way to run as per the README is to specify these in an extentions= parameter. If you don't do this, it defaults to just .php and .inc]

Ready for review.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Fixed

Merged, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • jonathan1055 authored 680f4808 on 8.3.x 
    fix(DisallowShortOpenTag): Exclude DisallowShortOpenTag from .yml files...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue tags: +Vienna2025

tagging.

Status: Fixed » Closed (fixed)

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