BaseGroupedMapper::removeGroup doesn't remove contained form fields

sonata-project / SonataAdminBundle

The missing Symfony Admin Generator

https://docs.sonata-project.org/projects/SonataAdminBundle

MIT License

2.11k stars 1.26k forks source link

BaseGroupedMapper::removeGroup doesn't remove contained form fields #8076

Closed s-bernstein closed 1 year ago

s-bernstein commented 1 year ago

Although removeGroup ist calling the remove method in

https://github.com/sonata-project/SonataAdminBundle/blob/b288b7b1a4a8f8241b3f19faa5ef247627e8f6a2/src/Mapper/BaseGroupedMapper.php#L271:L273

the form fields are not removed. The remove method expects the array key, not the value.

VincentLanglet commented 1 year ago

Same exists for removeTab.

Could you provide the PR @s-bernstein ?

s-bernstein commented 1 year ago

I probably could, but I won't for several reasons.

First, it's not a complicated fix. It is way more time efficient if someone, who already has a devel eveironment add the array_keys method to the appropriate code lines. This will take less than 5 minutes. If I have to clone, setup, edit, commit, push and create a PR, that will take me the better part of half an hour - so no.

Second, even if I would put up with that, the result will be, that there are additional task to cope with, like CI. And then, this PR will remain a PR for at least the next 3 years.

So, please create the PR yourself. Thank you.

jordisala1991 commented 1 year ago

I probably could, but I won't for several reasons.

First, it's not a complicated fix. It is way more time efficient if someone, who already has a devel eveironment add the array_keys method to the appropriate code lines. This will take less than 5 minutes. If I have to clone, setup, edit, commit, push and create a PR, that will take me the better part of half an hour - so no.

Second, even if I would put up with that, the result will be, that there are additional task to cope with, like CI. And then, this PR will remain a PR for at least the next 3 years.

So, please create the PR yourself. Thank you.

I did write like 3-4 replies to this. But I guess I will do the same as you. It will take too much time to explain all the things wrong in your answer to Vincent, so better think about it yourself and come again later when you have figured it out.

javiereguiluz commented 1 year ago

@s-bernstein I can understand that you think this change takes too much effort from you ... but please, be considerate of the people behind the open source projects that you use.

Writing things like "and then, this PR will remain a PR for at least the next 3 years" is unnecessarily cruel. Sonata project maintainers are volunteers and they do their best to review and merge the community contributions. They are part of the group of "unsung heroes" who make open source possible. Let's thank them for their dedication and let's try to help them as much as we can. Thanks!

s-bernstein commented 1 year ago

Oh... I have figured it out already. Last time I found a bug in Sonata Admin, I used about 8 hours of my time to fix it. First I supplied change, that solved the problem, but was a rather quick and dirty hack. I then changed in a way suggested by some member. And after all that was done - nothing. No reaction. No replies. For 3 years. And then, out of the blue, someone of the devel team contacted me about that PR, because it doesn't merge with the new version anymore and asks, if I could patch it for the new version. It still isn't merged. I'm still using my own patch locally (which is the hacked one because it only involved a single file).

And yes, I know, you are all volunteers - guess what, so am I, and is everybody who ever supplied a PR or a bug report for that matter. So, respect to ALL volunteers.

jordisala1991 commented 1 year ago

Think again. I will give you a clue: With volunteers like you, I prefer you to fork Sonata and maintain your own version.

That being said, isn't that the good thing about Open Source? The code is there, you just have to take it :)

s-bernstein commented 1 year ago

There... #8077 is the PR to this problem. I can't wait...

jordisala1991 commented 1 year ago

There... #8077 is the PR to this problem. I can't wait...

You cant wait for what... do you think that this issue with all the complains (For a change that is 2 line long) is going to help you in any way in reducing the review and accepting process of Sonata?

s-bernstein commented 1 year ago

No, I don't expect anything.

s-bernstein commented 1 year ago

Thank you for confirming it.

7ochem commented 1 year ago

@s-bernstein I myself am also working with Sonata and have supplied a few fixes in PRs here and there. And it's true that supplying a PR takes time and most of the time you are asked to change things and it does feel like a lot of work sometimes. That is absolutely true and I feel your frustration. However the level of respectlessness for others you are showing here is really intolerable. You are doing yourself no good with this attitude: it won't help getting your issues further and it is damaging the cooperation with others here or anywhere else. My advice to you: Next time be kind to others and be a little more patient. This will get you much further...