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

Why autoload param disabled for class_exists in Compiler.php ? #115

Closed
4n70w4 opened this issue Nov 19, 2019 · 16 comments
Closed

Why autoload param disabled for class_exists in Compiler.php ? #115

4n70w4 opened this issue Nov 19, 2019 · 16 comments

Comments

@4n70w4
Copy link

4n70w4 commented Nov 19, 2019

Case: I use composer and every request your library recompiled all classes.

if (class_exists($aopClassName, false)) {

        if (class_exists($aopClassName, false)) {
class_exists ( string $class_name [, bool $autoload = TRUE ] ) : bool

This function checks whether or not the given class has been defined.

class_name
The class name. The name is matched in a case-insensitive manner.

autoload
Whether or not to call __autoload by default.

ytake/Laravel-Aspect#63

@koriym
Copy link
Member

koriym commented Nov 20, 2019

This is because AOP class is not compiled yet.

Related with #114 . I will explain how to boost the performance in README.

@4n70w4
Copy link
Author

4n70w4 commented Nov 20, 2019

Simple patch fix this issue:

Index: vendor/ray/aop/src/Compiler.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- vendor/ray/aop/src/Compiler.php	(date 1574169214940)
+++ vendor/ray/aop/src/Compiler.php	(date 1574169214940)
@@ -76,7 +76,7 @@
             return $class;
         }
         $aopClassName = $this->getAopClassName($class, $bind);
-        if (class_exists($aopClassName, false)) {
+        if (class_exists($aopClassName, true)) {
             return $aopClassName;
         }
         $this->requireFile($aopClassName, new ReflectionClass($class), $bind);

Can you explain or give examples in which the false argument really is needed here?

@koriym
Copy link
Member

koriym commented Nov 21, 2019

Did you confirm the autoloader works ?

@izayoi256
Copy link
Contributor

izayoi256 commented Nov 21, 2019

I'm still waiting an explanation about caching. But #116 seems to be simple and fundamental solution for me, so closed mine #114.

I confirmed that this is working correctly using autoload.classmap in composer.json. If the compiled file exists, Compiler::compile() returns the compiled class name before parsing.

    "autoload": {
        "classmap": [
            "path/to/compile/directory"
        ]
    },

@izayoi256
Copy link
Contributor

izayoi256 commented Nov 21, 2019

Once I compiled, It always loads them even if in a development environment tho.

It compiles only when the annotations are changed. it works well.

@koriym
Copy link
Member

koriym commented Nov 21, 2019

My original thought is class compile / cache is handled by "upper layer" like DI container or factory code. Compiler should be called only when it really needed, Like first appearance, Not on every request.

But yes, I will try with your solution but with better autoloading. ("classmap" needs composer dump on every new class added ?) Please review when ready. I'm already working on it.

@koriym
Copy link
Member

koriym commented Nov 21, 2019

For example, Ray.Di could avoid this issue. It calls compiler only when class generation needed. #155 adds unnecessary autoloader calls for Ray.Di.

@4n70w4
Copy link
Author

4n70w4 commented Nov 21, 2019

@koriym

Compiler should be called only when it really needed

This is designed by package https://github.com/ytake/Laravel-Aspect
https://github.com/ytake/Laravel-Aspect/blob/d8df608e14faa1e896be8b36eb9e47477f2144c5/src/RayAspectKernel.php#L113

I created issue ytake/Laravel-Aspect#63

I do not know how good this solution is and whether it can be improved.

@koriym
Copy link
Member

koriym commented Nov 21, 2019

Umm... Seems too much job on runtime.

@4n70w4
Copy link
Author

4n70w4 commented Nov 21, 2019

@koriym In this case, it seems to me that my patch is the perfect solution. #116

I did not find anything that negatively affects.

@koriym
Copy link
Member

koriym commented Nov 21, 2019

class_exists.php

<?php
namespace Ray\Aop;

function class_exists(string $class)
{
    return \class_exists($class, true);
}

This function is just a quick hack...
(You can override class_exists with this function file loading.)

@koriym
Copy link
Member

koriym commented Nov 21, 2019

I pushed #117.
Thought ?

@koriym
Copy link
Member

koriym commented Nov 22, 2019

@4n70w4 I would appreciate your comments about #117

@4n70w4
Copy link
Author

4n70w4 commented Nov 22, 2019

@koriym I do not understand what you are doing and what is happening in #117 ╮( ̄ω ̄;)╭

@koriym
Copy link
Member

koriym commented Nov 22, 2019

( ̄ω ̄;)╭ -> New Weaver class offers a better solution to boost performance.

@koriym
Copy link
Member

koriym commented Dec 3, 2019

Closed. Please use Weaver instead.

@koriym koriym closed this as completed Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants