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

[Docs - Field] Order of "disabled()" & "relationship()" #9869

Merged
merged 7 commits into from
Nov 26, 2023

Conversation

Ahmant
Copy link
Contributor

@Ahmant Ahmant commented Nov 25, 2023

Mention that it's necessary to define disabled() before relationship().

// ❌
SomeField::make('users')
    ->relationship('users', 'name')
    ->disabled()

// ✅
SomeField::make('users')
    ->disabled()
    ->relationship('users', 'name')

Because dehydrated() in relationship() will be overrided by the one in disabled()

// vendor\filament\forms\src\Components\Concerns\CanBeDisabled.php - Line 17
$this->dehydrated(fn (Component $component): bool => ! $component->evaluate($condition));

@Ahmant Ahmant changed the title [Docs - Field] Order of "disabled()" & "relationship" [Docs - Field] Order of "disabled()" & "relationship()" Nov 25, 2023
@danharrin danharrin added the documentation Improvements or additions to documentation label Nov 25, 2023
@danharrin danharrin added this to the v3 milestone Nov 25, 2023
@danharrin
Copy link
Member

Not all fields have relationship(), and not all relationship() call dehydrated(), so I'm not sure if this belongs on the Getting Started page. Should we add it multiple times on more specific pages?

@danharrin danharrin self-assigned this Nov 25, 2023
@Ahmant
Copy link
Contributor Author

Ahmant commented Nov 25, 2023

I see your point.

I this situation we only have 3 components with relationship(), and in all these 3 components the dehydrated() function is called, so adding that note 3 times will not be bad.

  • CheckboxList
    • Path: forms\src\Components\CheckboxList.php
    • Line: 194
  • Repeater
    • Path: forms\src\Components\Repeater.php
    • Line: 914
  • Select
    • Path: forms\src\Components\Select.php
    • Line: 984

And note that the order will make an issue only when $someCondition == true in disabled($someCondition), because in this case dehydrated will become false:

// filament\forms\src\Components\Concerns\CanBeDisabled.php
public function disabled(bool | Closure $condition = true): static
{
    $this->isDisabled = $condition;
 👉 $this->dehydrated(fn (Component $component): bool => ! $component->evaluate($condition));

    return $this;
}

Is this right or I'm missing something?

@Ahmant
Copy link
Contributor Author

Ahmant commented Nov 25, 2023

I'm unsure if it's worth adding this to the documentation, but it took me around 4 hours maybe to figure out. After upgrading from v2 to v3, I encountered an error when saving my record, which was caused by this incorrect order.

Strangely, it worked in v2 but not in v3 🤷‍♂️.

@danharrin
Copy link
Member

Let's add the note to those 3 separate pages then, instead of Getting Started

In v2, we didn't disable dehydration for disabled fields by default, thats why the issue wasn't there

@Ahmant
Copy link
Contributor Author

Ahmant commented Nov 26, 2023

Ok 👌, I will add them, and push them for review

@Ahmant
Copy link
Contributor Author

Ahmant commented Nov 26, 2023

I pushed the changes.

But I made a mistake in my previous statement.

And note that the order will make an issue only when $someCondition == true in disabled($someCondition), because in this case dehydrated will become false:

The dehydrated in the relationship() must be false (it is by default), so the order will make an issue when $someCondition == false in disabled($someCondition), because in this case dehydrated will become true:

@danharrin danharrin merged commit b40a45a into filamentphp:3.x Nov 26, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants