Refactor calling trait methods from repository

[ghobaty]

Context

In ModuleRepository methods like prepareFieldsBeforeCreate checks if a method with the same name exists on the loaded traits, and if it this is the case then the trait method(s) get called.

Issue

As an example, we wanted to extend/override one of the traits
\A17\Twill\Repositories\Behaviors\HandleTranslations e.g.

<?php

namespace App\Repositories\Traits;

use App\Helpers\I18n;

trait HandleTranslations
{
    use \A17\Twill\Repositories\Behaviors\HandleTranslations {
        orderHandleTranslations as originalHandleTranslations;
    }

    public function orderHandleTranslations($query, &$orders)
    {
        // some custom behavior
    }
}

Now, this code snippet

twill/src/Repositories/ModuleRepository.php

Lines 468 to 472 in 9eaedc4

	foreach (class_uses_recursive(get_called_class()) as $trait) {
	if (method_exists(get_called_class(), $method = 'prepareFieldsBeforeCreate' . class_basename($trait))) {
	$fields = $this->$method($fields);
	}
	}

would call prepareFieldsBeforeCreateHandleTranslations twice, and since prepareFieldsBeforeCreateHandleTranslations is not idempotent it messes up with the fields data.

Proposed Solution (this PR)

This PR makes sure that that the traits methods array is unique before calling them.

[ifox]

Hi @ghobaty, thank you so much for you contribution!

This indeed makes sense. I added a very minor performance improvement while reviewing by avoiding the overhead of the debug_backtrace function internally. I assume your intention was to avoid repeating the calling function name, so I used the __FUNCTION__ magic constant instead.

As far as the implementation, cc @yanhao-li, this is really a functional flavor of what was already happening, but include a unique check. As you identified, the same issue has been fixed in Laravel itself here.

If we were to use the same coding style, our new traitsMethods function would look like this:

$methods = [];

foreach (class_uses_recursive(get_called_class()) as $trait) {
    $traitMethod = $method . class_basename($trait);
    
    if (method_exists(get_called_class(), $traitMethod) && !in_array($traitMethod, $methods)) {
        $methods[] = $traitMethod;
    }
}

return $methods;

In this PR, the same logic is written with a functional approach, which I actually really like:

$traits = array_values(class_uses_recursive(get_called_class()));

$uniqueTraits = array_unique(array_map('class_basename', $traits));

$methods = array_map(function (string $trait) use ($method) {
    return $method . $trait;
}, $uniqueTraits);

return array_filter($methods, function (string $method) {
    return method_exists(get_called_class(), $method);
});

Some could argue that the former is more readable/understandable than the latter, and I think the functional approach should not be overused sometimes, but here it looks fine to me.