Skip to content

Implement isPublicSet(), fix isVirtual(), add isVirtual to parity tests (P3 + P5)#170

Merged
lisachenko merged 1 commit intomasterfrom
feature/php84-isPublicSet-isVirtual
Apr 27, 2026
Merged

Implement isPublicSet(), fix isVirtual(), add isVirtual to parity tests (P3 + P5)#170
lisachenko merged 1 commit intomasterfrom
feature/php84-isPublicSet-isVirtual

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot commented Apr 27, 2026

Summary

Implements two sub-tasks from #162 (PHP 8.3/8.4 parity meta-issue):

P3 (#165): isPublicSet() method

  • Adds isPublicSet() to ReflectionProperty that returns true when the set visibility is explicitly public(set) and the main visibility is not already public
  • Delegates to php-parser's isPublicSet() on the underlying Property/Param node
  • Note: native PHP 8.4's \ReflectionProperty does not have isPublicSet(), so parity testing is done via standalone test with expected values

P5 (#167): isVirtual() in parity test getters

  • Adds 'isVirtual' to the PHP 8.4 getters in getGettersToCheck() so the parity test compares output with native reflection

Bug fix: isVirtual() implementation

  • Adding isVirtual to parity tests exposed an incorrect implementation
  • Previous logic only checked byRef on hooks — now correctly detects backing store usage:
    • Short set hooks (set => expr) always use backing store → NOT virtual
    • Block-form hooks that reference $this->propertyName use backing store → NOT virtual
    • Uses NodeFinder to walk AST for $this->propertyName references in block-form hooks

Review & Testing Checklist for Human

  • Verify isPublicSet() semantics match your expectations — since native PHP lacks this method, the behavior is defined by us (returns false for all valid PHP 8.4 code since public(set) can't widen beyond main visibility)
  • Verify isVirtual() fix handles edge cases: short get-only hooks (virtual), short set hooks (not virtual), block set hooks with/without $this->prop reference
  • Run vendor/bin/phpunit — all 11807 tests should pass with 0 failures

Notes

  • PHPStan passes with 0 errors
  • PHPUnit: 11807 tests, 12519 assertions, 0 failures (141 skipped, 6 incomplete — pre-existing)

Link to Devin session: https://app.devin.ai/sessions/9af7c65d163e48af819f6c20c039d99b
Requested by: @lisachenko

- P3: Add isPublicSet() method to ReflectionProperty that returns true
  when the set visibility is explicitly public(set) and the main visibility
  is not already public. Includes standalone test since native PHP 8.4 does
  not have this method.

- P5: Add 'isVirtual' to the parity test getters for PHP 8.4.

- Fix isVirtual() to correctly detect backing store usage:
  - Short set hooks (set => expr) always use the backing store
  - Block-form hooks that reference $this->propertyName use the backing store

Refs #165, #167

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@lisachenko lisachenko merged commit e6909f8 into master Apr 27, 2026
7 checks passed
@devin-ai-integration devin-ai-integration Bot added this to the 4.0.0 milestone Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

1 participant