Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

[BC Break] Drop built-in code scanners for 4.0 #155

Open
lisachenko opened this issue Apr 19, 2018 · 3 comments
Open

[BC Break] Drop built-in code scanners for 4.0 #155

lisachenko opened this issue Apr 19, 2018 · 3 comments

Comments

@lisachenko
Copy link
Contributor

There is a discussion in the PR #123 about possible removing of low-level scanners in the Zend\Code\Scanner subnamespace for 4.0. Reason for that is to avoid keeping support for low-level code analysis via tokens inside scanners, this job can be performed by external libraries, such as nikic/php-parser, roave/better-reflection, goaop/parser-reflection, etc.

Some facts about Scanner code usage in the project #123 (comment):

  • There are only dependencies on Scanner namespace in the Reflection namespace, eg. Zend\Code\Reflection\DocBlockReflection::reflect uses DocBlockScanner class, as well Zend\Code\Reflection\FileReflection::reflect uses CachingFileScanner. All remaining integration parts could be removed as they unused.
  • Classes from the Reflection namespace adds some public API to the PHP internal classes, thus Reflection namespace could not be easily dropped if we want to support *Generator::fromReflection() methods that accept only Zend\Code\Reflection classes and use specific methods from reflection.
  • If we decide to drop fromReflection() method from all generators or replace typehint in them with built-in PHP classes, then whole Scanner and Reflection namespace could be dropped, but it will be significant BC break as we remove all functionality that are not related directly to the code generation.

@Ocramius suggestions from #123 (comment):

Whole Scanner namespace could be removed, there are only dependencies in Reflection namespace, eg. Zend\Code\Reflection\DocBlockReflection::reflect uses DocBlockScanner class, as well Zend\Code\Reflection\FileReflection::reflect uses CachingFileScanner. All remaining integration parts could be removed as they unused.

Yeah, this stuff can be dropped. At this point, pointing to roave/better-reflection for most of the "brute-forcing the filesystem, looking for symbols"

Classes from the Reflection namespace adds some public API to the PHP internal classes, thus Reflection namespace could not be easily dropped if we want to support *Generator::fromReflection() methods that accept only Zend\Code\Reflection classes and use specific methods from reflection.

Not sure about this one. I think the initial idea was that ext-reflection had huge potholes, and the library tried to fill them. I'd keep this for a separate issue, to figure out if the Zend\Code\Reflection namespace actually does anything more than ext-reflection these days. Considering that Zend\Code\Reflection is the entry point to all of the usages of this library (including the generators), I think that would make the migration path too painful at first.

If we decide to drop fromReflection() method from all generators or replace typehint in them with built-in PHP classes, then whole Scanner and Reflection namespace could be dropped, but it will be significant BC break as we remove all functionality that are not related directly to the code generation.

The ::fromReflection() constructors are extremely useful to mimick API without rewriting all of it manually, so I'd probably keep them in place. Instead, I think that allowing core reflection classes as parameters would be a better solution here.

I like the idea to drop both Scanner and Reflection namespaces from the code.

Let's start with the Scanner only for now.

@lisachenko
Copy link
Contributor Author

Yeah, this stuff can be dropped. At this point, pointing to roave/better-reflection for most of the "brute-forcing the filesystem, looking for symbols"

@Ocramius So, we are going to drop both DocBlockReflection and FileReflection classes, right? Or keep them but use external libs? For example, DocBlockReflection contains sensitive logic for accessing tags, however we can mimic this logic via optional dependency phpdocumentor/reflection-docblock that provides good API for that.

The ::fromReflection() constructors are extremely useful to mimick API without rewriting all of it manually, so I'd probably keep them in place. Instead, I think that allowing core reflection classes as parameters would be a better solution here.

Ok, I will check if it's possible to switch to the core classes without pain.

Let's start with the Scanner only for now.

Ok.

@Ocramius
Copy link
Member

So, we are going to drop both DocBlockReflection and FileReflection classes, right?

I think so, yes.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-code; a new issue has been opened at laminas/laminas-code#5.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants