Skip to content

Ignore expanded entity nodes in source-backed sniffs#32

Merged
jordikroon merged 2 commits into
php:mainfrom
NickSdot:stack-01-entity-expansion
Jul 22, 2026
Merged

Ignore expanded entity nodes in source-backed sniffs#32
jordikroon merged 2 commits into
php:mainfrom
NickSdot:stack-01-entity-expansion

Conversation

@NickSdot

Copy link
Copy Markdown
Contributor

Prevents sniffs from reporting violations for DOM nodes introduced by entity expansion rather than present in the inspected source file.

Depends on #29, #30, and #31.
This PR diff will shrink as preceding PRs are merged and this branch is rebased.

@NickSdot
NickSdot force-pushed the stack-01-entity-expansion branch from 6974c7c to 27bcd61 Compare July 21, 2026 08:44
@NickSdot
NickSdot marked this pull request as ready for review July 21, 2026 08:45
@NickSdot

Copy link
Copy Markdown
Contributor Author

Rebased and ready for review.

@jordikroon jordikroon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Before:
FOUND 114632 violation(s) (114632 error(s), 0 warning(s)) in 10370 file(s).

After:
FOUND 50759 violation(s) (50759 error(s), 0 warning(s)) in 9993 file(s).

Given that #33 will introduce --wide, this behaviour is expected. But good to know.

Comment thread phpunit.xml.dist
<testsuite name="unit">
<directory>tests/Unit</directory>
</testsuite>
<testsuite name="integration">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While it's debatable if they are unit tests. Could you merge them into tests/Unit instead?

@NickSdot NickSdot Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, can follow your preference! Before I move them back: the end result in later branches is having Unit, Feature, and Integration. That is because there will be tests that run actual git commands for e2e testing. Given that, still all to Unit?

Ref: https://github.com/NickSdot/php__docbook-cs/tree/stack-09-performance/tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ideally, yes. As it would also be more easy to distinguish between tests and non-tests (fixtures & Support).

Comment thread src/Runner/EntityExpansionMarker.php Outdated
@NickSdot
NickSdot force-pushed the stack-01-entity-expansion branch from c1980f3 to 4798a98 Compare July 22, 2026 05:37
@NickSdot

Copy link
Copy Markdown
Contributor Author

Before: FOUND 114632 violation(s) (114632 error(s), 0 warning(s)) in 10370 file(s).

After: FOUND 50759 violation(s) (50759 error(s), 0 warning(s)) in 9993 file(s).

Given that #33 will introduce --wide, this behaviour is expected. But good to know.

Ah, I should have mentioned this here. This is a change in behaviour that is not only related to --wide. Assume shared has violation on <foo> which is used in A and B, current main reports the same violation for A and B, although it only can be fixed in shared. The current behaviour in main is similar to what PHPStan does with traits; but in our situation that is not actually helpful coz a consumer cannot fix the violation by changing it's own code (unlike traits reported by stan). After this change these kind of violations are reported only once against shared in full or --wide runs. This gets relevant when fixers come in, coz a fixer cannot edit <foo> in expanded A coz <foo> exists in shared; having less overall reports is a side effect.

@jordikroon
jordikroon merged commit a80c4c8 into php:main Jul 22, 2026
3 checks passed
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.

2 participants