Macroable - Parent's macros get overriden by descendants

[StyleShit]

The current mechanism of the Macroable trait overrides the parent's macros when using the same name. Check this test for example:

class A {
    use Macroable;
}

class B extends A {}

it('should not override parent macros', function () {
    A::macro('test', function () {
        return 'A';
    });

    B::macro('test', function () {
        return 'B';
    });

    expect(A::test())->toBe('A');
    expect(B::test())->toBe('B');
});

The first assertion will fail. That's actually a side-effect of a bigger issue - The Macroable class and all of its descendants share the same macros array, which means any macro of a sub-class will override the parent's one (if they're using the same name), and will also be available for any descendant of the parent class (e.g. let's say we have classes B & C that both extend A. When adding a macro to C it'll be available to A, B & C).

I think the expected behavior is to be somewhat similar to pure OOP:

class A {
    use Macroable;
}

class B extends A {}

class C extends A {}

class D extends B {}

it('should override parent macros only for the current class and its descendants', function () {
    A::macro('test', function () {
        return 'A';
    });

    B::macro('test', function () {
        return 'B';
    });

    expect(A::test())->toBe('A');
    expect(B::test())->toBe('B');
    expect(C::test())->toBe('A');
    expect(D::test())->toBe('B');
});

it('should not add macros to other classes', function () {
    C::macro('macroC', function () {
        return 'C';
    });

    expect(function () {
        A::macroC();
    })->toThrow(BadMethodCallException::class);

    expect(function () {
        B::macroC();
    })->toThrow(BadMethodCallException::class);

    expect(C::macroC())->toBe('C');
});

I'm not sure how we can solve this (unless there is some black magic that I'm not aware of 😅).
The only way that came to my mind is to change the macros array structure a little bit so it won't be flat, maybe something like this:

protected static $macros = [
    'Namespace\Of\Class\A' => [
        'testMacro' => function () {
            return 'A';
        },
        'macroThatShouldBeInherited' => function () {
            return 'A';
        },
    ],
    'Namespace\Of\Class\B' => [
        'testMacro' => function () {
            return 'B';
        },
    ],
];

And then when calling the macro, we can use ReflectionClass::getParentClass() recursively to pull from the array.

I'm not sure that I like this approach, it feels kinda fragile & complicated because we need to implement inheritance ourselves, and using reflection might have some performance penalty (even though I've heard it's not that bad, but I don't really know).

Plus, it probably won't be backward compatible if someone (for some reason) accessed the macros array directly from outside of the trait.

Any ideas if and how we can solve this properly?

[henzeb]

It would be a laravel 10 solution for sure. 'accessing the macros array directly' shouldn't be worried about, accessing internals is a risk the developer takes on his own.

[henzeb]

Speaking of, I am currently working on a library that relies on this behavior. Maybe we could give it a switch, or add a method called oopMixin/oopMacro so the user can specify what behavior is to be expected.