Filament•2y ago

Render hooks are duplicated in grouped relation managers

When Grouped relation managers are used, renderhooks are duplicated in each subsequent relation manager as you can see in the screenshot. This only happens in grouped relation managers and only happens on first render. When clicking any button (be it in the renderhook view or in the table), the duplicated render hooks go away. If you have three relation managers the renderhooks will appear three times, etc. The issue seems to be in registerRenderHook in FilamentManager. Since grouped relation managers run a loop to output the tables, it duplicates the renderHooks views in the array. Just for ease of access, here is that method:

public function registerRenderHook(string $name, Closure $callback): void
{
    $this->renderHooks[$name][] = $callback;

    ray($callback);

    ray($this->renderHooks);
}

public function registerRenderHook(string $name, Closure $callback): void
{
    $this->renderHooks[$name][] = $callback;

    ray($callback);

    ray($this->renderHooks);
}

I've attached a screenshot of the ray dump showing the callback and the renderhooks where you can see the AddressRelationManager view being added again to the array. This isn't related to the content of the view. If you remove all the view content to just be a simple <div>Hello world</div> you still get the duplication. Not entirely sure how to fix this. If anyone has any suggestions, I can submit a PR.

19 Replies

Kenneth SeseOP•2y ago

I guess one option would be to check if the view already exists in the array and not add it. Not sure if that would have other consequences. Hmmmm…

awcodes•2y ago

Is the filter set specific to the table, or does it apply to all tables?

Kenneth SeseOP•2y ago

Per table. The renderhook is applied when the relationManager is booted. This is a scenario that I didn’t anticipate when I PRed the RelationManager renderhooks. This probably affects anyone using renderhooks in grouped relationManagers

awcodes•2y ago

Hmm. Might need to move the render hook. I know that would be a PR. Also not sure that would resolve the issue though. But I don’t know if a way to check for active tab since that’s handled by alpine on the front end.

Kenneth SeseOP•2y ago

Might be the only solution I’ll try that and see. Unfortunately moving the renderhook doesn't help. After looking into it, it makes sense that this doesn't work because the Filament::renderHook() is just outputting the array of views that the registerRenderHook() method is giving it. So this has to be fixed in the registerRenderHook method. The easy fix is to just overrid the array key. So instead of $this->renderHooks[$name][] = $callback; it'd be $this->renderHooks[$name][$name] = $callback; This "fixes" the issue in this particular case, BUT if there were more than one renderhook that needed to be registered (like 2 separate plugins were using the same renderhook), then this wouldn't work since only the last one would show. So this isn't a fix. However, it raises the question: what DOES happen when more than one plugin use the same renderhook? There is no way to sort render hooks at the moment. I guess they just rendered by class name??? Anyway, just a separate question/thought I had.

awcodes•2y ago

Yea. Definitely not a solid way to handle that. You’d kinda need some kind of way to prioritize them. WP does something similar with their hooks.

Kenneth SeseOP•2y ago

Yes Filament might need some sort of way to do that. It doesn't see that this has been an issue for anyone at the moment...or at least I haven't seen help issues about...but I feel like it's just a matter of time before it is. I'm going to have to leave this as is for now. More and more I feel this might be a livewire problem so this might get fixed in v3. And just for a POC (and to show this isn't a plugin thing), you can add the following to two relation managers, group them, and you'll see Hello World repeated:

public function boot()
    {
        Filament::registerRenderHook(
            'resource.relation-manager.start',
            fn () => 'Hello World',
        );
    }

public function boot()
    {
        Filament::registerRenderHook(
            'resource.relation-manager.start',
            fn () => 'Hello World',
        );
    }

awcodes•2y ago

Same problem with assets too. Probably needs to be a way to force loading them in case they depended on other assets being loaded first. Ie loading styles after core styles.

wyChoong•2y ago

Your table header toolbar PR probably also suffer from this same issue: no specificity

Kenneth SeseOP•2y ago

Don’t know what you mean by “specificity”, but yes you are probably right. Any renderhooks inside grouped relation managers will probably have this problem.

wyChoong•2y ago

As you don’t know which items in the $this->renderHooks[$name][] array should be displayed

Kenneth SeseOP•2y ago

I’ll check later on and will add a comment to the PR so the team is aware

wyChoong•2y ago

Because I did imagine not all table/relation manager need the same hook If that can be solved then issue probably can be solved

Kenneth SeseOP•2y ago

Yes I think the team will have to restructure renderhooks so they work with grouped relation managers. But not just that but also when there are more than one thing using the same renderhook.

wyChoong•2y ago

Maybe something like isset($this->renderHooks[$name.$componentName]), on mobile so not going to type proper example

Kenneth SeseOP•2y ago

Yes that is what I had suggested too, but I think this would break if multiple plugins used the same renderhook.

wyChoong•2y ago

Nope it’s different that yours because I’m suggesting component name append to the render hook name So when you dump the array, you have keys like resource.relation-manager.start and resource.relation-manager.start.list-customer-contact-table Just throwing out ideas. Seems need more refactoring to get the whole idea works 😅

Kenneth SeseOP•2y ago

I like the idea. I’ll try to see if something like this can work. 👍 @Dan Harrin I wanted to loop you in here now because I have a potential fix for this that I wanted your input on before going forward with testing and PRing. You can read the discussion above with @wychoong, but TLDR, grouped relation managers duplicate renderhooks. The fix I have is to add the component name to the render hook to distinguish between them and then to only output that component's render hooks. So in practice: We would register render hooks like this:

public function boot()
{
    Filament::registerRenderHook(
        'resource.relation-manager.start',
        $this->getName(), // Added this
        fn () => 'Render hook stuff',
    );
}

public function boot()
{
    Filament::registerRenderHook(
        'resource.relation-manager.start',
        $this->getName(), // Added this
        fn () => 'Render hook stuff',
    );
}

Which then get stored like this:

public function renderHook(string $name, string $component, Closure $callback): static
{
    // added the $component key
    $this->renderHooks[$name][$component][] = $callback;

    return $this;
}

public function renderHook(string $name, string $component, Closure $callback): static
{
    // added the $component key
    $this->renderHooks[$name][$component][] = $callback;

    return $this;
}

And then retrieved:

public function getRenderHook(string $name, string $component): Htmlable
{
    $hooks = array_map(
        fn (callable $hook): string => (string) app()->call($hook),
        $this->renderHooks[$name][$component] ?? [],
    );

    return new HtmlString(implode('', $hooks));
}

public function getRenderHook(string $name, string $component): Htmlable
{
    $hooks = array_map(
        fn (callable $hook): string => (string) app()->call($hook),
        $this->renderHooks[$name][$component] ?? [],
    );

    return new HtmlString(implode('', $hooks));
}

And finally rendered:

@php
    // Each renderHook would need to get the component
    $component = $this->getName();
@endphp
<div class="filament-resource-relation-manager">
    {{ filament()->renderHook('resource.relation-manager.start', $component) }}

    {{ $this->table }}

    {{ filament()->renderHook('resource.relation-manager.end', $component) }}
</div>

@php
    // Each renderHook would need to get the component
    $component = $this->getName();
@endphp
<div class="filament-resource-relation-manager">
    {{ filament()->renderHook('resource.relation-manager.start', $component) }}

    {{ $this->table }}

    {{ filament()->renderHook('resource.relation-manager.end', $component) }}
</div>

This at least works in grouped relation managers, and I suspect it would work everyplace else, but you have a mile-high view of everything so maybe there's something I'm overlooking. This would require all render hooks to be updated. There might be tests that need to be updated as well? And maybe part of the upgrade script? Maybe you have a better way to accomplish this? And with v3 around the corner, honestly this might not be high priority. As mentioned, this issue ONLY affects render hooks in grouped relation managers. Finally, the other piece discussed above is the ability to sort render hooks if more than one component/plugin is using the same one. But I feel that could wait.

Dan Harrin•17mo ago

Implemented this in v3 beta, registerRenderHook('name', hook, scope: 'optional scope, usually the LW component')

Gaming

Programming

Render hooks are duplicated in grouped relation managers