- 🇪🇸Spain plopesc Valladolid
This is a useful feature we have been using for a while.
It also includes tests.
Marking as RTBC.
- Status changed to Needs work
over 1 year ago 8:04am 12 September 2023 - 🇳🇱Netherlands megachriz
- I see that there is a method added to the unit test class, but the feature that is provided here isn't actually tested. No test is calling
getPluginWithKey()
. - A case needs to be added to Drupal\Tests\tamper\Functional\Plugin\Tamper\ExplodeTest::formDataProvider() to ensure that the new setting gets saved. Existing cases need to be adjusted.
- The description of the setting could use an example. I had to inspect the code to understand what the setting was doing. Maybe change the description into "If enabled, the keys and values of the array will be the same: @code.". And "@code" will then be the code example, encapsulated in code-tags.
- I see that there is a method added to the unit test class, but the feature that is provided here isn't actually tested. No test is calling
- Status changed to Needs review
over 1 year ago 7:43am 14 September 2023 - last update
over 1 year ago 489 pass - 🇳🇿New Zealand ericgsmith
Thank you for the patches and reviews.
It also took me a while to understand what was expected based on the description. It feels like maybe this isn't something the explode plugin needs to be doing? But it seems like there is a valid use case here.
Attaching a patch to include this as an additional plugin rather than introducing anything new into the explode plugin. Patch includes update to the chained tamper test to show them working together.
This approach adds a little bit of complexity for the user who now has to add 2 plugins, but if this functionality is needed when using the explode plugin then it could well be useful in other contexts where we have an array from another source that is not the explode plugin.