Skip to content

Conversation

W0rma
Copy link
Contributor

@W0rma W0rma commented Aug 14, 2025

Description

Follow-up of #82

Type of change

setAccessible() is only called if PHP < 8.1 is used because it is noop as of PHP 8.1.

This is useful because a deprecation will be triggered in PHP 8.5 if this method is used (see https://wiki.php.net/rfc/deprecations_php_8_5#extreflection_deprecations).

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • Unit tests
  • Spec Tests
  • Integration tests / Manual Tests

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@RikudouSage
Copy link
Collaborator

I don't think this is necessary, it's simply a noop on newer versions, meaning it's optimized away, while adding an if seems like unnecessary branching, even if simple.

Once support for older versions is dropped, it will be removed completely, until then I think keeping it as is is the best course of actions.

@jdreesen
Copy link

But it triggers a deprecation in PHP 8.5.

@RikudouSage
Copy link
Collaborator

I see, I added a simple comment and then we can merge it.

@W0rma W0rma force-pushed the reflection-set-accessible-php81 branch from 7b0c283 to 30de441 Compare August 15, 2025 09:20
@github-project-automation github-project-automation bot moved this from New to Approved PRs in Issues and PRs Aug 15, 2025
@RikudouSage RikudouSage merged commit ec6c2ad into Unleash:main Aug 15, 2025
2 checks passed
@github-project-automation github-project-automation bot moved this from Approved PRs to Done in Issues and PRs Aug 15, 2025
@W0rma W0rma deleted the reflection-set-accessible-php81 branch August 15, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants