[TwigComponent] PreMount VS required props

[cavasinf]

First time using TwigComponent, but works like a charm good job 👍
I've stumbled upon a case that I can't accomplish without feeling like "it's too much code".

Simple case:

• Required props title.
• Pass Entity to the props which is Stringable.
• Component used like this:

  <twig:DropdownHeaderItem title="{{ app.user }}"/>

Here's the Component:

// src/Twig/Components/DropdownHeaderItem.php

#[AsTwigComponent]
final class DropdownHeaderItem extends DropdownItem
{
    public string $title;
}

And his twig:

{# templates/components/DropdownHeaderItem.html.twig #}

<h6
        {{ attributes }}
        class="dropdown-header {{ this.extraClass }}" {{ macros.attr_to_html(this.attr) }}
>
    {{ this.title }}
</h6>

Sadly, this is not enough to make the title option required AND with an explicit error message.
If you call the component with no parameters:

<twig:DropdownHeaderItem/>

This is the error that is displayed:

Two problems:

1. It show the error INSIDE his component template and not where it is used.
2. The stack trace (as usual with twig) doesn't show where the error occured in twig (only controller with ->render()...)

Looking at the doc, we should use PreMount hook with the OptionsResolver like this:

#[AsTwigComponent]
final class DropdownHeaderItem extends DropdownItem
{
    public string $title;

    #[PreMount]
    public function preMount(array $data): array
    {
        $resolver = new OptionsResolver();
        $resolver->setRequired('title');
        $resolver->setAllowedTypes('title', ['string', \Stringable::class]);

        return $resolver->resolve($data);
    }
}

I'm not a fan of that because:

1. Nowadays we can do a lot with only PHP8 (declarations/types/attributes) for simple cases.
   PHP auto __toString() call if property typed to string when class implements the method.
2. The string reference 'title' to the property $title (maintainability).
3. You need to declare ALL other properties, otherwise you'll get The option "xxxx" does not exist
4. All the default value from the PHP declaration, need to be redefined IN the OptionsResolver.
5. +9 lines of code just for a required.

Is there any limitation, to not be able to print required error without the OptionResolver?

[smnandre]

Your property is typed string, so even if you allow stringable in the option, it wont be set during mount without some code i think.

What surprises me is that the error is not thrown earlier.

What happens if :

• you give explicitly a string to the component
• you cast your title during mount

public function mount(string|\Stringable $title): void
{
      $this->title = (string) $title;
}

[cavasinf]

What surprises me is that the error is not thrown earlier.

All of the code above (from my example), doesn't trigger any error though.

An error is thrown if the title argument is missing,
but with a message that is not explicit at all if there is no OptionsResolver

[smnandre]

So you suggest to improve the error message, right ? Or is the "stringable" thing more your point there ?
i'm asking to be certain to understand the main issue :)

What would do you suggest the message 'say' ?

[cavasinf]

Yes, it's more of a question/improvement than a bug (I should have been more/less specific, sorry).

The main question is:
Is there any limitation to not be able to define component usage without the OptionResolver (preMount)?

Can't we define the component configuration from the component property itself?
Like entity bound to Symfony Form:

• Required (nullable or not)
• Type
• Default value
• Allowed values (perhaps from an enum)
• Visible/Settable (public or private + Symfony\UX\TwigComponent\Attribute\ExposeInTemplate)

[cavasinf]

For example, what the OptionsResolver tells "more" than the property definition:

#[AsTwigComponent]
final class Alert
{
    public string $message;

    #[PreMount]
    public function preMount(array $data): array
    {
        $resolver = new OptionsResolver();
        $resolver->setIgnoreUndefined();

        $resolver->setRequired('message');
        $resolver->setAllowedTypes('message', 'string');

        return $resolver->resolve($data);
    }
}

• Required: because string not nullable and NO default value

• Type: string as defined by the type

• Default value: as defined directly in the PHP property

   #[AsTwigComponent]
   final class Alert
   {
       public string $type = 'success';
   
       #[PreMount]
       public function preMount(array $data): array
       {
           $resolver = new OptionsResolver();
           $resolver->setIgnoreUndefined();
   
           $resolver->setDefaults(['type' => 'success']);
   
           return $resolver->resolve($data);
       }
   }

• Allowed values can be defined through an Enum:

  enum ColorEnum: string
  {
      case Primary        = 'primary';
      case Secondary      = 'secondary';
  
      case Success      = 'success';
      case Warning      = 'warning';
      case Danger       = 'danger';
      case Info         = 'info';
  }

  #[AsTwigComponent]
  final class Alert
  {
      public ColorEnum $color;
  
      #[PreMount]
      public function preMount(array $data): array
      {
          $resolver = new OptionsResolver();
          $resolver->setIgnoreUndefined();
  
          $resolver->setRequired('color');
          $resolver->setAllowedValues('color', [
              'primary',
              'secondary',
              'success',
              'warning',
              'danger',
          ]);
  
          return $resolver->resolve($data);
      }
  }

[smnandre]

LiveComponent uses validator and constraints ... could we find a way to use it there ?

What make me doubts regarding the OptionsResolver is that it will not work with LiveComponent and i would like to avoid users needing to have two parralel validation systems ..

[WebMamba]

I agree here! With an anonymous component when you set a variable without a default variable, you get an error telling you that this prop is required to use this component. But with a standard class component, you can't make your props mandatory just by not setting a default variable. I think it could be a nice improvement but this is a huge BC break! What we can do in a PR is add an option strictPropsif this option is set to true we throw an error if a component is used without giving a props with no default value. WDYT?

[smnandre]

What if..... we introduce a "Prop" argument ... that could be some "LiveProp" but for TwigComponent ?

That would allow us to add type, constraints, hydration, ...

[WebMamba]

Do we really need this argument? What about public props but not with the Prop argument? What do you mean by type?

[smnandre]

Just thinking out loud / suggesting something to handle the BC :)

[cavasinf]

I haven't tested the LiveComponent, so I don't measure the impact of these changes (but I understand the BC break).

As @WebMamba said, coming from the anonymous component, I was "chocked" by all the changes that I have to do to convert it to a TwigComponent BUT when converting it to a LiveComponent will be maybe easier? 🤔

The complexity of the TwigComponent is maybe due to the fact that it was made from/for the LiveComponent (with validator, constraints and other).

---

To handle the BC break:

• strictProps is nice and simple, but it means EVERY public property follows these rules without exception.
• The Prop argument is more tedious and harder to deprecate, but more "configurable".

If I have to choose, I'll go with strictProps.
Today it will be defaulted to false, and in the next major release it will be defaulted to true.
We can already deprecate the strictProps when using the default value by saying you should explicitly define it to be false.

@WebMamba Have you considered of the impact/conflict with Symfony\UX\TwigComponent\Attribute\ExposeInTemplate. Is there any?

[smnandre]

Hmm.. really not sure about this.

The prop argument has two advantages:

• beeing easy to migrate into LiveProp one
• explicitly tag the prop as beeing ... a prop

And that would allow some Validation trait (as in Live component) .. and use real constraint attribute tags (instead or this config resolve that i personaly do not find user friendly at all

coming from the anonymous component, I was "chocked" by all the changes that I have to do to convert it to a TwigComponent

I would love to have more insights about this !

[cavasinf]

As the doc says about LiveProp:

Most properties will be LiveProps

So in our case, most properties should be Prop?

Why not do the opposite, everything is prop except when you add an attribute or just a private declaration?
That is why I liked the strictProps proposal, you can set everything with ONE boolean, and handle the BC break + deprecation from one boolean too.

I understand the pain as a bundle developer 😄, but I also see the improvement for developers using Symfony 💨.

As an example of a HUGE improvement in the quality of life for developers, is the mapping ORM for entity.
We've come from:

  #[ManyToOne(targetEntity: Address::class)]
  #[JoinColumn(name: 'address_id', referencedColumnName: 'id')]
  private Address|null $address = null;

to:

  #[ManyToOne]
  private ?Address $address = null;

[smnandre]

Yes... with an attribute 👼