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

Having a grid migration tool #272

Open
mamazu opened this issue Nov 1, 2022 · 4 comments
Open

Having a grid migration tool #272

mamazu opened this issue Nov 1, 2022 · 4 comments

Comments

@mamazu
Copy link
Member

mamazu commented Nov 1, 2022

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 Sylius/Sylius#14449 (comment)


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
Copy link
Member

loic425 commented Nov 1, 2022

@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
Copy link
Contributor

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
Copy link
Member Author

mamazu commented Nov 4, 2022

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
Copy link
Member Author

mamazu commented Nov 5, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants