Re: [PHP-DEV] ReflectionContext for imports and namespace

[Andreas Hennings]

On 11 December 2017 at 09:23, Andreas Hennings  wrote:
>>> These side effects would be that the class loader loads files which can
>>> break things?
>>
>>
>> Yes. Reflecting over a codebase which contains even just polyfills
>> (duplicate classes) can already lead to unexpected crashes. Reflecting over
>> non-PSR-2 code can even lead to worse things such as starting a DB
>> connection and performing unwanted operations.
>>
>
> My own use case avoids this problem.
> Instead of randomly scanning anything, you need to tell the annotation
> parser explicitly which namespace directory you want to scan.
> It is the caller's responsibility that all class files in this
> directory are nicely shaped, and can be safely autoloaded.
>
> In fact I did use a userland parser in the past, but then decided that
> native reflection is safe with the above assumption.


Maybe one fragile edge case would be if one of the files causes the
autoloader to include a different file, outside of the library, due to
ambiguous namespace mapping.
But this problem would also occur during everyday use of the library,
if this class is used.

So the condition is: Only scan namespace directories where you know
that every class can be safely autoloaded.
In fact I do not explicitly include the class file, but let the class
loader do this, to be sure that the same version of the class is used
that would be used in a regular request / process.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionContext for imports and namespace

[Andreas Hennings]

>> These side effects would be that the class loader loads files which can
>> break things?
>
>
> Yes. Reflecting over a codebase which contains even just polyfills
> (duplicate classes) can already lead to unexpected crashes. Reflecting over
> non-PSR-2 code can even lead to worse things such as starting a DB
> connection and performing unwanted operations.
>

My own use case avoids this problem.
Instead of randomly scanning anything, you need to tell the annotation
parser explicitly which namespace directory you want to scan.
It is the caller's responsibility that all class files in this
directory are nicely shaped, and can be safely autoloaded.

In fact I did use a userland parser in the past, but then decided that
native reflection is safe with the above assumption.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionContext for imports and namespace

[Marco Pivetta]

On 11 Dec 2017 09:10, "Andreas Hennings"  wrote:



On 11 December 2017 at 09:05, Marco Pivetta  wrote:

> On 11 December 2017 at 08:46, Marco Pivetta  wrote:
> > Indeed that already exists at
> > https://github.com/Roave/BetterReflection/blob/2.0.1/docs/fe
> atures.md#analysing-types-from-docblocks
> > - relatively new lib, so it probably didn't get noticed upfront in here.
>
>
> Yes, parser / userland solutions exist for this purpose.
> (I have seen BetterReflection)
>
> I just thought since this information is already available, a library
> that uses reflection API should not need a userland parser to get it.
>
>
> Unless the codebase being analyzed is trusted and not legacy
> (wordpress-style) any tool based on the current reflection API is basically
> a potential security issue or a set of potentially harmful side-effects.
> The reason for me and James building BetterReflection was essentially that,
> since the current API is flawed and not really fixable without BC breaks
> (removing the side-effect), so I strongly encourage any code analysis tool
> to just use the userland adapters we wrote, and only switch to core
> reflection when performance is more critical than security.
>

These side effects would be that the class loader loads files which can
break things?


Yes. Reflecting over a codebase which contains even just polyfills
(duplicate classes) can already lead to unexpected crashes. Reflecting over
non-PSR-2 code can even lead to worse things such as starting a DB
connection and performing unwanted operations.

Re: [PHP-DEV] ReflectionContext for imports and namespace

[Andreas Hennings]

On 11 December 2017 at 09:05, Marco Pivetta  wrote:

> On 11 December 2017 at 08:46, Marco Pivetta  wrote:
> > Indeed that already exists at
> > https://github.com/Roave/BetterReflection/blob/2.0.1/docs/
> features.md#analysing-types-from-docblocks
> > - relatively new lib, so it probably didn't get noticed upfront in here.
>
>
> Yes, parser / userland solutions exist for this purpose.
> (I have seen BetterReflection)
>
> I just thought since this information is already available, a library
> that uses reflection API should not need a userland parser to get it.
>
>
> Unless the codebase being analyzed is trusted and not legacy
> (wordpress-style) any tool based on the current reflection API is basically
> a potential security issue or a set of potentially harmful side-effects.
> The reason for me and James building BetterReflection was essentially that,
> since the current API is flawed and not really fixable without BC breaks
> (removing the side-effect), so I strongly encourage any code analysis tool
> to just use the userland adapters we wrote, and only switch to core
> reflection when performance is more critical than security.
>

These side effects would be that the class loader loads files which can
break things?


>
> This also means that if your addition makes it into the language it will
> be implemented also by those adapters BTW, so push on 
>

Re: [PHP-DEV] ReflectionContext for imports and namespace

[Marco Pivetta]

On 11 December 2017 at 08:46, Marco Pivetta  wrote:
> Indeed that already exists at
> https://github.com/Roave/BetterReflection/blob/2.0.1/
docs/features.md#analysing-types-from-docblocks
> - relatively new lib, so it probably didn't get noticed upfront in here.


Yes, parser / userland solutions exist for this purpose.
(I have seen BetterReflection)

I just thought since this information is already available, a library
that uses reflection API should not need a userland parser to get it.


Unless the codebase being analyzed is trusted and not legacy
(wordpress-style) any tool based on the current reflection API is basically
a potential security issue or a set of potentially harmful side-effects.
The reason for me and James building BetterReflection was essentially that,
since the current API is flawed and not really fixable without BC breaks
(removing the side-effect), so I strongly encourage any code analysis tool
to just use the userland adapters we wrote, and only switch to core
reflection when performance is more critical than security.

This also means that if your addition makes it into the language it will be
implemented also by those adapters BTW, so push on 

Re: [PHP-DEV] ReflectionContext for imports and namespace

[Andreas Hennings]

>
> Documentation tools shouldn't run the code IMO, that means they won't have
> access to that feature.

By "documentation tools" do you mean libraries like phpDocumentor?
You are right, those need to parse anyway, they cannot use reflection API.

But tools for annotation discovery may want to use native reflection
instead of parsing, it is an implementation choice.


>
> Nikic's PHP parser already contains a tool that resolves all namespaces, you
> can probably write a similar visitor for the Ast that also works for PHPdoc
> blocks.
>
> Regards, Niklas

On 11 December 2017 at 08:46, Marco Pivetta  wrote:
> Indeed that already exists at
> https://github.com/Roave/BetterReflection/blob/2.0.1/docs/features.md#analysing-types-from-docblocks
> - relatively new lib, so it probably didn't get noticed upfront in here.


Yes, parser / userland solutions exist for this purpose.
(I have seen BetterReflection)

I just thought since this information is already available, a library
that uses reflection API should not need a userland parser to get it.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] ReflectionContext for imports and namespace

[Marco Pivetta]

Indeed that already exists at
https://github.com/Roave/BetterReflection/blob/2.0.1/docs/features.md#analysing-types-from-docblocks
- relatively new lib, so it probably didn't get noticed upfront in here.

It would probably be a good idea to address the fact that the current
reflection API causes autoloading and evaluation of the loaded files before
denying API improvement attempts upfront though: that limitation is
unrelated with what Andreas is proposing, in my opinion.

On 11 Dec 2017 08:36, "Niklas Keller"  wrote:

Andreas Hennings  schrieb am Mo., 11. Dez. 2017, 01:39:

> TLDR:
> I propose to introduce a new class \ReflectionContext (or perhaps
> \ReflectionScope?), to give information about imported aliases and the
> namespace.
>
>
> ## Background / motivation
>
> Some libraries that parse doc comment annotations, e.g. phpDocumentor,
> need to know the class aliases and the namespace that are active in
> the place (scope) where the class, function or method is declared.
>
> E.g. phpDocumentor has this class:
>
> https://github.com/phpDocumentor/TypeResolver/blob/
552bf401d264a443819a66233932be6a23f59d78/src/Types/Context.php
>
> To get this information, they parse the PHP file where the class is
> defined:
>
> https://github.com/phpDocumentor/TypeResolver/blob/
552bf401d264a443819a66233932be6a23f59d78/src/Types/ContextFactory.php
>
> It would be much more pleasant if PHP would provide this information
> through the reflection API.
> A custom library should not need to do an expensive and fragile
> parsing job for information that is already known.
>
> Also, this external parsing might not cover all cases, depending how
> it is implemented. E.g. local namespace scopes in curly brackets. (Who
> uses those, btw?)
>
>
> ## Proposal
>
> Make information about class aliases (imported through use statements)
> and the active namespace available through the reflection API.
>
> $reflClass = new \ReflectionClass(C::class);
> $reflContext = $reflClass->getContext();
> $aliases = $reflContext->getAliases();
>
>
> ## Details
>
> class ReflectionContext {
>
>   function getNamespace() : string {..}
>
>   /**
>* @return string[]
>*   Format: $[$alias] = $qcn
>*/
>   function getAliases() : string[] {..}
>
>   /**
>* @return string|null
>*   The qcn of the class, or null if it is a built-in type like
> "string".
>*/
>   function resolveAlias($alias) : ?string {..}
> }
>
> The $reflClass->getContext()->getNamespace() is the same as
> $reflClass->getNamespace().
>
> But having it all in the $context object allows this object to be
> passed around to components that need it.
>
>
> ## Open questions
>
> In a method doc comment, we also want to resolve the "self" keyword.
> For this, the class name is needed as well, not just external imports
> and namespace.
>
> Maybe also introduce \RefllectionMethod::getContext()? Not sure.
>
> Should it be "context" or "scope"? It is not the same. Maybe "scope"
> leads to the wrong expectations.
>

Documentation tools shouldn't run the code IMO, that means they won't have
access to that feature.

Nikic's PHP parser already contains a tool that resolves all namespaces,
you can probably write a similar visitor for the Ast that also works for
PHPdoc blocks.

Regards, Niklas

>

Re: [PHP-DEV] ReflectionContext for imports and namespace

[Niklas Keller]

Andreas Hennings  schrieb am Mo., 11. Dez. 2017, 01:39:

> TLDR:
> I propose to introduce a new class \ReflectionContext (or perhaps
> \ReflectionScope?), to give information about imported aliases and the
> namespace.
>
>
> ## Background / motivation
>
> Some libraries that parse doc comment annotations, e.g. phpDocumentor,
> need to know the class aliases and the namespace that are active in
> the place (scope) where the class, function or method is declared.
>
> E.g. phpDocumentor has this class:
>
> https://github.com/phpDocumentor/TypeResolver/blob/552bf401d264a443819a66233932be6a23f59d78/src/Types/Context.php
>
> To get this information, they parse the PHP file where the class is
> defined:
>
> https://github.com/phpDocumentor/TypeResolver/blob/552bf401d264a443819a66233932be6a23f59d78/src/Types/ContextFactory.php
>
> It would be much more pleasant if PHP would provide this information
> through the reflection API.
> A custom library should not need to do an expensive and fragile
> parsing job for information that is already known.
>
> Also, this external parsing might not cover all cases, depending how
> it is implemented. E.g. local namespace scopes in curly brackets. (Who
> uses those, btw?)
>
>
> ## Proposal
>
> Make information about class aliases (imported through use statements)
> and the active namespace available through the reflection API.
>
> $reflClass = new \ReflectionClass(C::class);
> $reflContext = $reflClass->getContext();
> $aliases = $reflContext->getAliases();
>
>
> ## Details
>
> class ReflectionContext {
>
>   function getNamespace() : string {..}
>
>   /**
>* @return string[]
>*   Format: $[$alias] = $qcn
>*/
>   function getAliases() : string[] {..}
>
>   /**
>* @return string|null
>*   The qcn of the class, or null if it is a built-in type like
> "string".
>*/
>   function resolveAlias($alias) : ?string {..}
> }
>
> The $reflClass->getContext()->getNamespace() is the same as
> $reflClass->getNamespace().
>
> But having it all in the $context object allows this object to be
> passed around to components that need it.
>
>
> ## Open questions
>
> In a method doc comment, we also want to resolve the "self" keyword.
> For this, the class name is needed as well, not just external imports
> and namespace.
>
> Maybe also introduce \RefllectionMethod::getContext()? Not sure.
>
> Should it be "context" or "scope"? It is not the same. Maybe "scope"
> leads to the wrong expectations.
>

Documentation tools shouldn't run the code IMO, that means they won't have
access to that feature.

Nikic's PHP parser already contains a tool that resolves all namespaces,
you can probably write a similar visitor for the Ast that also works for
PHPdoc blocks.

Regards, Niklas

>

[PHP-DEV] ReflectionContext for imports and namespace

[Andreas Hennings]

TLDR:
I propose to introduce a new class \ReflectionContext (or perhaps
\ReflectionScope?), to give information about imported aliases and the
namespace.


## Background / motivation

Some libraries that parse doc comment annotations, e.g. phpDocumentor,
need to know the class aliases and the namespace that are active in
the place (scope) where the class, function or method is declared.

E.g. phpDocumentor has this class:
https://github.com/phpDocumentor/TypeResolver/blob/552bf401d264a443819a66233932be6a23f59d78/src/Types/Context.php

To get this information, they parse the PHP file where the class is defined:
https://github.com/phpDocumentor/TypeResolver/blob/552bf401d264a443819a66233932be6a23f59d78/src/Types/ContextFactory.php

It would be much more pleasant if PHP would provide this information
through the reflection API.
A custom library should not need to do an expensive and fragile
parsing job for information that is already known.

Also, this external parsing might not cover all cases, depending how
it is implemented. E.g. local namespace scopes in curly brackets. (Who
uses those, btw?)


## Proposal

Make information about class aliases (imported through use statements)
and the active namespace available through the reflection API.

$reflClass = new \ReflectionClass(C::class);
$reflContext = $reflClass->getContext();
$aliases = $reflContext->getAliases();


## Details

class ReflectionContext {

  function getNamespace() : string {..}

  /**
   * @return string[]
   *   Format: $[$alias] = $qcn
   */
  function getAliases() : string[] {..}

  /**
   * @return string|null
   *   The qcn of the class, or null if it is a built-in type like "string".
   */
  function resolveAlias($alias) : ?string {..}
}

The $reflClass->getContext()->getNamespace() is the same as
$reflClass->getNamespace().

But having it all in the $context object allows this object to be
passed around to components that need it.


## Open questions

In a method doc comment, we also want to resolve the "self" keyword.
For this, the class name is needed as well, not just external imports
and namespace.

Maybe also introduce \RefllectionMethod::getContext()? Not sure.

Should it be "context" or "scope"? It is not the same. Maybe "scope"
leads to the wrong expectations.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php