Skip to content

Conversation

@Rafikooo
Copy link
Contributor

Initial error:
image

The error message did not clearly indicate what was wrong.

The issue was related to grid configuration merging.

In Sylius, we have:

sylius_grid:
    grids:
        fields:
            email:
                type: twig
                label: sylius.ui.email
                sortable: ~
                options:
                    template: "@SyliusAdmin/shared/grid/field/name.html.twig"

and in some end app:

sylius_grid:
    grids:
        fields:
            email:
                type: string
                label: sylius.ui.email
                sortable: ~
                position: 4

After merging, the type was set to string, but the template option from the base configuration remained, causing the application to break.

This PR introduces validation to prevent similar issues in the future, saving users from hours of debugging.

Comment on lines +301 to +303
/**
* @test
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* @test
*/
/** @test */

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use the PHP Attribute :)

/**
* @test
*/
public function it_throws_exception_when_field_template_is_used_with_non_twig_type(): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function it_throws_exception_when_field_template_is_used_with_non_twig_type(): void
public function it_throws_an_exception_when_field_template_is_used_with_non_twig_type(): void

@GSadee GSadee added Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). DX Issues and PRs aimed at improving Developer eXperience. labels Feb 14, 2025
->end()
->end()
->validate()
->ifTrue(fn (array $config): bool => isset($config['options']['template']) && $config['type'] !== 'twig')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an overreach that can plausibly cause conflicts with custom types.
Wouldn't it be better to reset the previous options once the type changes, we'd probably need to inject the logic somewhere inside the config merging mechanism though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants