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.

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

Transformers + Aspects: Somewhat incompatible #66

Open
WalterWoshid opened this issue Nov 5, 2023 · 0 comments
Open

Transformers + Aspects: Somewhat incompatible #66

WalterWoshid opened this issue Nov 5, 2023 · 0 comments
Labels
bug 🐞 Something isn't working

Comments

@WalterWoshid
Copy link
Contributor

WalterWoshid commented Nov 5, 2023

Using Code Transformers with Aspects has some problems.

Note: Transformers are always executed before Aspects.

Example of why it is incompatible:

class Target
{
    public function answer(): int
    {
        return 42;
    }
}

class Transformer extends TransformerClass
{
    // ...

    public function transform(Code $code): void
    {
        $sourceFileNode = $code->getSourceFileNode();

        // Find return type: 'int' ...
        $code->edit($node, 'int|float');

        // Find return value '42' ...
        $code->edit($node, "$nodeText.69");
    }
}

#[AspectAttribute]
class Aspect
{
    #[After(
        class: Target::class,
        method: 'answer',
    )]
    public function higherAnswer(AfterMethodInvocation $methodInvocation): int|float
    {
        $result = $methodInvocation->proceed();

        return $result + 378;
    }
}

$target->answer(); // Returns "420", should return "420.69"
  • First the transformer is applied, it changes the return type from int to int|float and changes the number 42 to 42.69
  • After that the aspect is applied, it adds 378 to the result, which should result to the final number 420.69
  • But it doesn't return 420.69, it returns 420
  • Why? Because the aspect builds the woven file only with the int type and not int|float
  • Why? Because the WovenClassBuilder uses the ReflectionMethod from the matching process (AspectMatcher->$matchedAdviceContainers) and not from the newly transformed file (After the transformer has been applied)

Here is how the woven class looks like:

class Target extends Target__AopProxied
{
	public function answer(): int // Should be int|float
	{
		return call_user_func_array(self::$__joinPoints['method']['answer'], [$this]);
	}
}

// Proxied class:
class Target__AopProxied
{
    public function answer(): int|float // Has int|float
    {
        return 42.69;
    }
}

Current workflow:

  • (In ClassLoader)
  • Match the aspects
  • Match the transformers (order doesn't matter)
  • Rewrite the file path to a StreamFilter which modifies the source code as a stream and at runtime. See PHP Stream Filters
  • Inside the StreamFilter the appliance of the transformers happens first, then the aspects are applied

How to fix this?

  • (In ClassLoader)
  • First match the transformers
  • Apply the transformers
  • Save the new class as a BetterReflection in a ReflectionContainer
  • Match the aspects by the new BetterReflection instance

Problems with this solution:

Usually first the matching is done, then the StreamFilter rewrites the file at runtime and this is where the appliance of both transformers and aspects happens. The StreamFilter provided the source code of the file as a stream.

Here the problem would be that we have to load the file manually when applying the transformers in the ClassLoader, which would be unnecessary.

Alternatively we could move the matching logic right into the StreamFilter, but then we'd have to figure out the namespace manually and every loaded class would end up in the StreamFilter, which would be a huge performance loss.

Do we even need Stream Filters?????

Don't even know why I am using them...

Yes, Xdebug will jump into the original file instead of debugging the proxied/woven files that are generated by this library, thanks to the Stream Filters. See #72 for more information.

@WalterWoshid WalterWoshid added the bug 🐞 Something isn't working label Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant