⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@X-Coder264
Copy link

We have this rule in our codebase (and other similar rules too):

    public function __invoke(): ArchRule
    {
        return Rule::allClasses()
            ->that(new RecursivelyExtend(EndToEndTestCase::class))
            ->andThat(new IsNotAbstract())
            ->should(new HaveNameMatching('*EndToEndTest'))
            ->because('all end-to-end tests must be named ending with EndToEndTest')
        ;
    }

Basically we want to make sure that all test class names in our testsuite that extend the base EndToEndTestCase class that are not abstract classes themselves must end with EndToEndTest.

After #454 this no longer works as the logic early returns in \Arkitect\Rules\Specs::allSpecsAreMatchedBy because \Arkitect\Expression\ForClasses\IsNotAbstract::appliesTo returns false for final classes (and all our test classes are final) and because of that the \Arkitect\Expression\ForClasses\HaveNameMatching expression never gets evaluated so no violations are reported.

After this fix appliesTo would return true and everything would continue to work as it did prior to that #454 PR.

@fain182
Copy link
Collaborator

fain182 commented Jan 24, 2026

Thanks for the pr!
I suppose we need to think a general solution for this problem...
The appliesTo for negative rules should have different behavior when used in a "given"...

fain182 pushed a commit that referenced this pull request Jan 25, 2026
This test replicates the issue described in PR #560 where IsNotAbstract
incorrectly filters out final classes when used in a that() clause.

The test creates a scenario with:
- Final classes with correct/incorrect naming
- Abstract classes (should be filtered out)
- Normal classes with incorrect naming

Expected behavior: IsNotAbstract should include final classes in the
filter since final classes are, by definition, non-abstract.

Current behavior: Test fails with 1 violation instead of expected 2,
demonstrating that final classes are incorrectly excluded.
fain182 pushed a commit that referenced this pull request Jan 25, 2026
This fixes the issue described in PR #560 where IsNotAbstract incorrectly
excluded final classes when used in that() clauses.

Root cause: appliesTo() was implementing two different semantics:
1. Type filtering: "Does this property apply to this type?"
2. Condition checking: "Does this class match the condition?"

This caused final classes to be excluded from IsNotAbstract filters, even
though final classes ARE non-abstract by definition.

Changes:
- IsAbstract/IsNotAbstract: Remove isFinal() check from appliesTo()
  Final classes can be checked for abstractness (they're always non-abstract)

- IsFinal/IsNotFinal: Remove isAbstract() check from appliesTo()
  Abstract classes can be checked for finality (they're always non-final)

New semantics for appliesTo():
- Returns true if the property can be semantically verified for this type
- Only excludes interface/trait/enum where abstract/final concepts don't apply
- Does NOT exclude mutually exclusive states (abstract/final)

All 350 tests pass, including new integration tests that verify:
- IsNotAbstract in that() now includes final classes
- IsNotAbstract in should() correctly validates all class types
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