Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Switch to php documentor reflection #44

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

refactor: Switch to php documentor reflection #44

wants to merge 2 commits into from

Conversation

DannyvdSluijs
Copy link
Contributor

@DannyvdSluijs DannyvdSluijs commented Jan 9, 2021 •
edited
Loading

This draft pull request shows a rough implementation of how php-advanced-json-rpc could use PhpDocumentor Reflection. In order to resolve #9.
I've added this PR to have a more constructive discussion about the pros and cons. I've based this on the latest stable mayor version of PHPDocumentor Reflection (^4.0). That version only support PHP 7.2 and up (Which is causing the build to fail). This already raises some concern as this lib is aiming to support PHP 7.0 but that is already two years EOL (See https://www.php.net/supported-versions.php). So perhaps some updates are needed for this lib as well?

This PR has the following changes:

  • Extracts the current implementation, based on the native reflection classes of PHP, into an interface (strategy pattern)
  • Implements a PhpDocumentor Reflection implementation of above interface

In order to complete this PR the following is needed:

  • Add unit tests for the new implementation
  • Decide if the current approach with the Reflection abstraction layer is something that is desired
  • Resolve any @todo added in this PR
  • General cleanup of the PR.
  • Ensure if we have the optimal way of caching implemented in order to resolve Cache more reflection objects #2

@DannyvdSluijs DannyvdSluijs force-pushed the Switch-to-php-documentor-reflection branch from 8006920 to e269399 Compare January 11, 2021 20:05
@DannyvdSluijs DannyvdSluijs changed the title Switch to php documentor reflection refactor: Switch to php documentor reflection Jan 11, 2021
@codecov
Copy link

codecov bot commented Jan 11, 2021 •
edited
Loading

Codecov Report

Merging #44 (e269399) into master (06f0b06) will decrease coverage by 15.11%.
The diff coverage is 62.96%.

@@              Coverage Diff              @@
##             master      #44       +/-   ##
=============================================
- Coverage     89.07%   73.95%   -15.12%     
- Complexity       58       96       +38     
=============================================
  Files             8       13        +5     
  Lines           119      192       +73     
=============================================
+ Hits            106      142       +36     
- Misses           13       50       +37
Impacted Files Coverage Δ Complexity Δ
lib/Reflection/PhpDocumentorReflection.php 0.00% <0.00%> (ø) 16.00 <16.00> (?)
lib/Reflection/NativeReflection.php 90.24% <90.24%> (ø) 22.00 <22.00> (?)
lib/Dispatcher.php 87.50% <90.90%> (-1.39%) 18.00 <0.00> (-10.00)
lib/Reflection/Dto/Method.php 100.00% <100.00%> (ø) 4.00 <4.00> (?)
lib/Reflection/Dto/Parameter.php 100.00% <100.00%> (ø) 4.00 <4.00> (?)
lib/Reflection/Dto/Type.php 100.00% <100.00%> (ø) 2.00 <2.00> (?)
... and 1 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Switch to phpDocumentor/reflection Cache more reflection objects
1 participant
@DannyvdSluijs