Having a grid migration tool

Sylius / SyliusGridBundle

Generic data grids for Symfony applications, support Doctrine and custom drivers, sorting, filtering, actions and field types.

MIT License

118 stars 49 forks source link

Having a grid migration tool #272

Open mamazu opened 2 years ago

mamazu commented 2 years ago

That's a very good initiative, we'll be the refactor team of Sylius :) As we said, that'd be cool to use a converter that users could use to convert their own grids. A non-official one exists and maybe we could just try it and improve it and maybe Moving it to the grid package.

Originally posted by @loic425 in https://github.com/Sylius/Sylius/issues/14449#issuecomment-1280041599

I would like to offer to take that topic on. The question is what except for the namespace would need to be changed for the converter to be merge into the official Sylius bundle?

loic425 commented 2 years ago

@mamazu That's a good news :) @Florian-Merle Have you tested the tool? I don't have tested myself yet. So I cannot answer for now.

But @mamazu If you can add it on a pull-request with PHPUnit tests, I think we'll be able to be sure everything will be ok for end-users.

Florian-Merle commented 2 years ago

The tool works great :). However, during my work on the conversion of the sylius grids from YAML to PHP, I have not written down what can be improved, so I might not be exhaustive in the following list.

The tool does generate a lot of useless imports (but with the right IDE/tools, it's not that big of an issue)
It does not add a line break between methods (I guess it could be nice to have, but it's not a must have IMO)
I'm not sure about this one, but the generated grid has a a static method definition for getResourceClass but I think it's a mistake
There is no way to generate a grid that implements ResourceAwareGridInterface

That being said, with the right tools (I'm thinking of php-cs-fixer), it is quite easy to generate grids. Also a generated grid is almost already ready to be used :)

mamazu commented 2 years ago

Coincidentally the last two points have already been addressed in the newest version of the tool.

The only issue we have is, that by default it generates grids assuming the underlying entity is a resource so that might be something that we don't want.

As for the first two points, I completely agree and the first point is also still on the todo list for the tool.

I am not sure if I should put in time to fix the second point. Because I think if you are using Sylius you more than likely already have ecs set up. So it would just be another command you would run before committing anyways, but good catch.

mamazu commented 2 years ago

First point is mostly addressed in the Merge Requested version of the tool, I will sadly not be backporting this to my repository.

This is due to the fact that the MR version of the tool has a different structure.