[TwigComponent] Missleading doc between Passing & Rendering Attributes and PreMount validation

[cavasinf]

Rendering "extra" Attributes with a PreMount hook + OptionsResolver will not work if followed as the doc explains it.

As from the example, if the Alert component looks like this:

#[AsTwigComponent]
final class Alert
{
    public string $message;
    public string $type = 'success';

    #[PreMount]
    public function preMount(array $data): array
    {
        // validate data
        $resolver = new OptionsResolver();
        $resolver->setDefaults(['type' => 'success']);
        $resolver->setAllowedValues('type', ['success', 'danger']);
        $resolver->setRequired('message');
        $resolver->setAllowedTypes('message', 'string');

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

Calling it like this:

<twig:Alert id="alert_id" message="My message"/>

Will throw an UndefinedOptionsException error:

Caution

Error rendering "Alert" component: The option "id" does not exist. Defined options are: "message", "type".

We may need to resolve this incoherence by:

1. Add an extra paragraph in the PreMount hook part, to explain how to do it with extra attributes.
2. Re-write the "example" preMount function doc like this:

#[PreMount]
public function preMount(array $data): array
{
    // validate data
    $resolver = new OptionsResolver();
+   $resolver->setIgnoreUndefined();
+
    $resolver->setDefaults(['type' => 'success']);
    $resolver->setAllowedValues('type', ['success', 'danger']);
    $resolver->setRequired('message');
    $resolver->setAllowedTypes('message', 'string');

-   return $resolver->resolve($data);
+   return array_merge($data, $resolver->resolve($data));
}

---

I could have done a PR directly, but I want to talk about it first and discuss if it is necessary or not.

[smnandre]

Do you need the array_merge ?

As multiple preMount are allowed, i'd just add a commented line in the code block

#[PreMount]
public function preMount(array $data): array
{
    // validate data
    $resolver = new OptionsResolver();
    
+   // Uncomment following line to allow extra attributes
+   // $resolver->setIgnoreUndefined();
+

wdyt ?

[cavasinf]

I understand the problem with the multiple PreMount... 🤯

When using IgnoreUndefined, all data not defined through the options are ignored:
https://github.com/symfony/symfony/blob/a8b4739a2a9c70508509b93468c0c0d38e64e1c0/src/Symfony/Component/OptionsResolver/OptionsResolver.php#L881-L883

So the resolve result will never contains those "extra data".

---

Do you need the array_merge ?

With the same Alert from above,
the attributes from the component are:

[
  "id" => "alert_id",
  "message" => "My message",
]

OptionsResolver resolve it to:

[
  "type" => "success",
  "message" => "My message",
]

Without the array_merge we lost the id.
It should be:

[
  "id" => "alert_id",
  "message" => "My message",
  "type" => "success",
]

wdyt ?

Maybe we need an extra PreMount Hook with different priority when using OptionsResolver and extra attributes? 🤔

Edit:

Just tested with two PreMount hook, and the priority doesn't matter.
The $data from the hook is the result of the previous hook OR if first all the attributes from the component.
if $resoler->resolve(...) always remove the id attribute, being first or last doesn't matter.

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

    $resolver->setDefaults(['type' => 'success']);
    $resolver->setAllowedValues('type', ['success', 'danger']);
    $resolver->setRequired('message');
    $resolver->setAllowedTypes('message', 'string');

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

#[PreMount(2)]
public function extraData(array $data): array
{
    return $data;
}

Result to:

[
  "type" => "success",
  "message" => "My message",
]

[smnandre]

And could a final PostMount (it should receive the additional data i think) be used ?

[cavasinf]

a final PostMount

Ok you pointed out an even more weirder case 😆

PostMount hook receive any data that doesn't match any property (or mount() argument).
BUT if PreMount is present and resolves data, which means removing any data that is not an option,
PostMount will get the result of the OptionsResolver.
BUT (once again) if PreMount has array_merge, PostMount will receive any data not handled by the OptionsResolver.

PostMount alone

#[AsTwigComponent]
final class Alert
{
    public string $message;
    public string $type = 'success';

    #[PostMount]
    public function postMount(array $data): array
    {
        dump($data);

        return $data;
    }
}

PostMount $data:

[
  "id" => "alert_id"
]

With PreMount and resolve only

#[AsTwigComponent]
final class Alert
{
    public string $message;
    public string $type = 'success';

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

        $resolver->setDefaults(['type' => 'success']);
        $resolver->setAllowedValues('type', ['success', 'danger']);
        $resolver->setRequired('message');
        $resolver->setAllowedTypes('message', 'string');

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

    #[PostMount]
    public function postMount(array $data): array
    {
        dump($data);

        return $data;
    }
}

PostMount $data:

[]

With PreMount and resolve with array_merge

#[AsTwigComponent]
final class Alert
{
    public string $message;
    public string $type = 'success';

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

        $resolver->setDefaults(['type' => 'success']);
        $resolver->setAllowedValues('type', ['success', 'danger']);
        $resolver->setRequired('message');
        $resolver->setAllowedTypes('message', 'string');

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

    #[PostMount]
    public function postMount(array $data): array
    {
        dump($data);

        return $data;
    }
}

PostMount $data:

[
  "id" => "alert_id"
]

[smnandre]

🤯

[cavasinf]

At the end.. Do we:

1. Say it is normal to array_merge
2. Change something in the PreMount hook
3. Change something in the PostMount hook
4. Change something in OptionsResolver

[Kocal]

That's also a pattern I've introduced in our codebase, however we use + over array_merge for performances reason (it uses less opcodes, see https://3v4l.org/sR78P/vld vs https://3v4l.org/i7DEg/vld):

    #[PreMount]
    public function preMount(array $data): array
    {
        $optionsResolver = new OptionsResolver();
        $optionsResolver
            ->setIgnoreUndefined()
            ->setRequired(['id'])
            ->setAllowedTypes('id', 'string')
        ;

        return $optionsResolver->resolve($data) + $data;
    }

If we keep the point of merging data/options, I would be in favor to document + usage instead of array_merge.

[cavasinf]

(it uses less opcodes, see https://3v4l.org/sR78P/vld vs https://3v4l.org/i7DEg/vld)

Good catch.

If we keep the point of merging data/options, I would be in favor to document + usage instead of array_merge.

I agree.

Do we add a new paragraph about "extra data" inside the PreMount part , or do we default the "If you need to modify/validate data" example with this code?

[WebMamba]

So this issue points out multiple things we should work on:

• improve the doc when using PreMount with extra attributes
• add a way to keep the extra attribute through the PreMount hook
• make sure the extra attribute are still available in the PostMount hook

Do I miss something?

[cavasinf]

make sure the extra attribute are still available in the PostMount hook

I think PostMount should work as it should if PreMount does not remove this extra data.

Do I miss something?

ATM, no 👍

[WebMamba]

@cavasinf are you up for doing these PRs? I think we should do this in two PRs one for the doc and one for the PreMount hook. You can say no, there is no pressure 😁

[cavasinf]

I'm actually in R&D on the Live Components AND Turbo (since the SymfonyLive thanks to your demo @WebMamba 😉) which is now stable.
I have 1 day per week dedicated to it, so it will take time. Depends if you want to have it fixed soon or not.

[WebMamba]

Great there is no hurry! It can be next week

[cavasinf]

With the current state, OptionsResolver will also remove arguments for mount().
So something like this will not work:

#[AsTwigComponent]
final class Alert
{
    public string $message;
    public string $type = 'success';

    public function mount(bool $isSuccess): void // <--- This argument without default value
    {
        $this->type = $isSuccess ? 'success' : 'danger';
    }

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

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

Used like that:

<twig:Alert id="alert_id" :isSuccess="false" message="My message"/>

Will produce:

Caution

Error rendering "Alert" component: App\Twig\Components\Alert::mount() has a required $isSuccess parameter. Make sure this is passed or make give a default value.

[smnandre]

Ok so this seems expected

->setIgnoreUndefined avoids throwing an exception, but ... ignores the undefined options. So resolver->resolve will not pass "isSuccess"

And the data you return from PreMount is the data passed to the mount() method.

---

So if i go back to your first example

#[AsTwigComponent]
final class Alert
{
    public string $message;
    public string $type = 'success';

    #[PreMount]
    public function preMount(array $data): array
    {
        // validate data
        $resolver = new OptionsResolver();
        $resolver->setDefaults(['type' => 'success']);
        $resolver->setAllowedValues('type', ['success', 'danger']);
        $resolver->setRequired('message');
        $resolver->setAllowedTypes('message', 'string');

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

You want to

• validate some data (type and message here)
• allow other key/values in data

So i suggest to do this

#[AsTwigComponent]
final class Alert
{
    public string $message;
    public string $type = 'success';

    #[PreMount]
    public function preMount(array $data): array
    {
        // validate data
        $resolver = new OptionsResolver();
        $resolver->setAllowedValues('type', ['success', 'danger']);
        $resolver->setRequired('message');
        $resolver->setAllowedTypes('message', 'string');

       // This will throw as you expect if type/message is invalid
       $resolver->resolve($data);
       
        // So here you can return all data (and keep your ID)
        return $data;
    }
}

WDYT ?

[cavasinf]

WDYT ?

No Bueno.

First, if the component is used with the id attribute, the OptionsResolver will throw an UndefinedOptionsException.
Second, the default data inside the OptionsResolver will never be sent to the component.

I've made a PR that simply concat those data variables.
At the end, you only need to add $resolver->setIgnoreUndefined(); and return the resolved options.

[smnandre]

Oops i forgot the line indeed ^^

Update:

#[AsTwigComponent]
final class Alert
{
    public string $message;
    public string $type = 'success';

    #[PreMount]
    public function preMount(array $data): array
    {
        // validate data
        $resolver = new OptionsResolver();
        $resolver->setIgnoreUndefined();
                
        $resolver->setAllowedValues('type', ['success', 'danger']);
        $resolver->setRequired('message');
        $resolver->setAllowedTypes('message', 'string');

       // This will throw as you expect if type/message is invalid
       $resolver->resolve($data);
       
        // So here you can return all data (and keep your ID)
        return $data;
    }
}

[cavasinf]

Second, the default data inside the OptionsResolver will never be sent to the component.

Still missing that part I think 🤔

[smnandre]

But the default values should be in your properties, no ? What would be the advantage of defining them in the resolver ?

[cavasinf]

What would be the advantage of defining them in the resolver ?

You can and the doc says to, so people will 🤷‍♂️

the default values should be in your properties

This is exactly why I've asked the question in this "issue".

[smnandre]

Oh, I didn't mean it as a criticism. I was just thinking aloud, trying to come up with a positive solution.

And i 100% agree with you at least the doc should be updated :)

[cavasinf]

Oh, I didn't mean it as a criticism

I wasn't either, sorry if I misspoke

[smnandre]

So remains this one =)

[cavasinf]

So remains this one =)

Yeah 😄
TBH, the end goal is to avoid this OptionsResolver thing.

For today's "how to use" it missing from the doc:

• the setIgnoreUndefined
• and return $optionsResolver->resolve($data) + $data;

Do not use:

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

As it will ignore the default values from OptionsResolver (which at the end is not recommended to use by @smnandre)