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

[Bug]: Cannot use Closure for fields with itemType() #196

Closed
mallardduck opened this issue Mar 5, 2024 · 3 comments · Fixed by #197
Closed

[Bug]: Cannot use Closure for fields with itemType() #196

mallardduck opened this issue Mar 5, 2024 · 3 comments · Fixed by #197
Labels
bug Something isn't working

Comments

@mallardduck
Copy link
Contributor

What happened?

I cannot currently use a Closure as the fields value when calling SkyPlugin::make()->itemType(). There are many reasons someone might want to customize a Form field using a closure as documented in filament docs. When I attempt doing this I get the following error:

Filament\Forms\Components\Component::getChildComponents(): Return value must be of type array, Closure returned

Flareapp trace of error: https://flareapp.io/share/x5MW6QoP

How to reproduce the bug

When I register SkyPlugin using custom itemType with a closure for fields (which Filament supports):

                SkyPlugin::make()
                    ->itemType(
                        name: "Test",
                        fields: function(): array {
                            return [
                                Select::make('test')
                                    ->label("Test")
                                    ->options([
                                        '' => "placeholder",
                                        'test' => "test"
                                    ])
                                    ->default('')
                                    ->selectablePlaceholder(false),
                            ];
                        },
                        slug: 'test'
                    ),

Package Version

3.X

PHP Version

8.1

Laravel Version

10.x

Which operating systems does with happen with?

macOS, Windows, Linux

Notes

Cannot sort out yet if this is a filament issue, or a Sky issue. Leaning towards sky issue related to how we're reusing the Filament Form Group - but could totally be something else upstream in filament I guess.

@mallardduck mallardduck added the bug Something isn't working label Mar 5, 2024
@atmonshi
Copy link
Member

atmonshi commented Mar 6, 2024

ya I think I am allowing to pass a closure but never evaluate it

can you test this for me see if works?
in the Configuration class

public function itemType(string $name, array | Closure $fields, ?string $slug = null): static
    {
        $this->itemTypes[$slug ?? Str::slug($name)] = [
            'name' => $name,
            'fields' => $this->evaluate($fields),
        ];

        return $this;
    }

also dont remember if there is other places need to change too

@mallardduck
Copy link
Contributor Author

so I gave this a try and while it did fix the error, it executes at a different time than expected. for instance, the specific new "itemType" i'm trying to add is "Local Route" - one that lists public (only having web middleware) routes not needing route parameters to create links from.

Initially I was hacking this up as a PR to submit to sky and it seemed to be working as I expected there. But then I realized that was overkill since the itemType method existed, so started to port it into my project.

the gist is that when I had the code in Sky it gets executed a lot "closer" to when the user clicks the "Add Link" button - so it properly can see all the apps routes. But when I moved it into itemType via my project it seems to run "at Panel configuration" time which runs before all the routes are registered.

@mallardduck
Copy link
Contributor Author

mallardduck commented Mar 6, 2024

Oh wait - I got it now. Instead of evaluating the fields "at configure time", we just do that "at call time" within getItemTypes and get the same fix with more of the app state online when evaluated. Edit: on third look, you already cover other instance of this in HandlesNavigationBuilder::class, so that is most appropriate for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants