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

bootMetable called for each child class, staking the same hooks numerous times #252

Open
Levivb opened this issue Oct 16, 2019 · 0 comments

Comments

@Levivb
Copy link

Levivb commented Oct 16, 2019

Hey,

Thanks for these great packages.

I found something to what might be seen as a bug when registering the hooks in eloquence-metable.

when working with the following structure:

abstract class Base {
    use Metable;
}

class ChildA {
}

class ChildB {
}

class ChildC {
}

while instantiating the child classes, each of 'm runs bootMetable. Which ofcourse makes sense given Laravel's bootTraits nature.

The problem arises in sofa/hookable/src/Hookable.php:27

    public static function hook($method, Closure $hook)
    {
        static::$hooks[$method][] = $hook;
    }

since the hooks are stored in a static $hooks property on the Base class, each child class shares the same property.
When instantiating 3 childclasses, all 3 childclasses will register the hooks in it's base class. Effectively duplicating the code which needs to run.

In our case, we had 200 child classes, so 200 hooks were registered in the base class. When retrieving a property, and thus running through the registered hooks of getAttribute, this caused quite a bit of performance loss and also a stack trace too deep.

I've fixed in on our side via:

use Sofa\Eloquence\Metable as MetableBase;

class Base {
    use MetableBase {
        MetableBase::bootMetable as metableBootMetable;
    }

    /** @var bool */
    private static $isMetableBooted = false;

    public static function bootMetable(): void
    {
        if (self::$isMetableBooted) {
            return;
        }

        self::$isMetableBooted = true;

        static::metableBootMetable();
    }

but it might be worthwhile to backport that fix it here

Regards,

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

1 participant