- Issue created by @angrytoast
- last update
10 months ago 29,469 pass - @angrytoast opened merge request.
- Status changed to Needs review
10 months ago 3:05pm 25 August 2023 - Status changed to RTBC
10 months ago 11:40pm 26 August 2023 - 🇺🇸United States smustgrave
Phpstorm even flags this as duplicates about to be immediately overridden.
- last update
10 months ago 29,469 pass - Status changed to Needs work
10 months ago 7:51am 27 August 2023 - 🇳🇱Netherlands Lendude Amsterdam
Can we get an analysis as to WHY this exists? Usually this is due to some old refactor that was done wrong and there should actually be two cases. So it would be nice to know if we broke something a long the way or if we just forgot to remove code. Having the history might give us an idea if this needs a follow up to maybe restore lost functionality or might indicate more left over code we can remove.
The test coverage for the grouped filters is sketchy at best, so that coming back green whenever this got in doesn't tell the whole story I fear.
- 🇺🇸United States angrytoast Princeton, NJ
As best as I can tell by looking at git blame, this happened in https://git.drupalcode.org/project/drupal/-/commit/40d5f16f5577d072228b9... for https://www.drupal.org/project/views/issues/731662 → , specifically between patch 129 and patch 136.
- 129 didn't have the duplication yet:
- https://www.drupal.org/project/views/issues/731662#comment-5682370 →
- https://www.drupal.org/files/731662-hybrid-filters.129.patch →
- By 136, about 2 months later, the duplication was introduced in the re-rolled patch in
function build_group_form - https://www.drupal.org/project/views/issues/731662#comment-6033342 →
- https://www.drupal.org/files/731662.views_.136.patch →
I didn't see any comments between the 2 points that suggests this should have be 2 separate code paths; given that it was a re-roll and the size of the patch, it seems like an unintentional code duplication.
- Status changed to RTBC
10 months ago 6:11am 28 August 2023 - 🇳🇱Netherlands Lendude Amsterdam
@angrytoast thanks for finding this and researching the origins, that is some pretty old code :) Back to RTBC
- last update
10 months ago 29,469 pass - last update
10 months ago 29,470 pass - last update
10 months ago 29,470 pass - last update
10 months ago 29,471 pass - last update
10 months ago 29,471 pass - last update
10 months ago 29,471 pass - last update
10 months ago 29,471 pass - last update
10 months ago 29,473 pass 27:37 24:24 Running- last update
9 months ago 29,477 pass - last update
9 months ago 29,477 pass - last update
9 months ago 29,482 pass - last update
9 months ago 29,482 pass - last update
9 months ago 29,482 pass - last update
9 months ago 29,483 pass - Status changed to Needs work
9 months ago 5:02am 26 September 2023 - 🇳🇿New Zealand quietone New Zealand
This doesn't apply to 11.x. Can someone update the MR? Thanks
- last update
9 months ago 30,151 pass, 115 fail - last update
9 months ago 30,363 pass - Status changed to Needs review
9 months ago 7:15pm 26 September 2023 - last update
9 months ago 30,363 pass - Status changed to RTBC
9 months ago 10:15pm 26 September 2023 -
quietone →
committed e3e45ab3 on 11.x
Issue #3383270 by angrytoast, smustgrave, Lendude: Remove duplicate code...
-
quietone →
committed e3e45ab3 on 11.x
- Status changed to Fixed
9 months ago 11:03pm 26 September 2023 - 🇳🇿New Zealand quietone New Zealand
Thanks for updating the MR.
@angrytoast, thank you for doing the research on how the duplicated code was introduced. That was really helpful. In the future, it would help even more to link to that information from the Issue Summary. It isn't such a burden on the reviewer to read 12 comments but on longer issues it can be challenging.
It is worth noting that the duplication was added in a reroll that did not have an accompanying diff or interdiff.
Committed e3e45ab and pushed to 11.x. Thanks!
Automatically closed - issue fixed for 2 weeks with no activity.