Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The only started using the _extract_ plugin recently and it works great. However, say, you run an array_filter on an array and remove empty values. The value then becomes:
[
1 => "Foo",
2 => "Bar",
5 => "Baz",
]
It gets difficult to get the first value (shift) or the last value (pop). Here are some proposed solutions:
- Have a parameter named "method" which can be set to "index", "pop", "shift"
- Have separate parameters named "index", "shift", "pop" - the first one found is evaluated
Comment | File | Size | Author |
---|---|---|---|
#26 | 2881779-26.patch | 6.89 KB | jigarius |
#26 | interdiff-24-26.txt | 1.66 KB | jigarius |
Comments
Comment #3
jigariusComment #4
jigariusComment #5
jigariusOops. Changing back to 8.5.x.
Comment #6
jigariusComment #7
jigariusComment #8
maxocub CreditAttribution: maxocub as a volunteer and commentedI think we already can do that with the callback plugin:
or
Comment #9
heddnGood work there, but I think I agree with #8.
Comment #10
jigariusThanks for the feedback! Two things to note here:
Please feel free to re-open if that makes sense.
Comment #11
jigariusI just tried to do the thing suggested in #8 and I remembered why I had to create a separate plugin called ArrayShift in my project in the first place. When you try to call array_pop or array_shift using the "callback" plugin, you get this message:
Hence, I'm reopening this issue. I think this is quite a common use case because I've stumbled upon such a problem 2 times in less than 3 months. I will see if I can write some tests for this this weekend.
Comment #12
heddnLet's make a few new process plugins for push and pop. And these should go into migrate_plus for now, until we find a use for them in core. Or at least incubate.
Comment #13
jigariusCreated 2 plugins - array_pop and array_shift. Do I need to include tests as well?
Comment #14
heddnYup, process plugins should test the edge cases and any exceptions. What happens if the data isn't an array, etc
Comment #15
jigariusDone. Added tests for both the shift and pop plugins.
Comment #16
jigariusBTW, thinking aloud, I just thought of something different. Is there a reason why the callable plugin doesn't call the callable with arguments by reference? What if we had something like this in the callable plugin?
That way, people will be able to call a plethora of callbacks without having to write new plugins. Example: array_pop, array_shift, sort and all other functions which expect arguments by reference instead of value. Any thoughts? Also, I think the callback process plugin should throw an exception when the callback doesn't exist or is not callable instead of silently doing nothing. Let me know if this sounds good and I'll create a patch, with tests, etc to get the wheel rolling. I'm feeling highly motivated this week (:
Comment #17
heddnre #16: We don't want callable to become essentially the php module. If it isn't super simple, then write a process plugin and create some tests. It isn't hard.
Open a core issue for the exception throwing. That seems like a reasonable suggestion.
Now for a review of #15
I think an example where we provide some sample values of what is in 'array' would help. Plus, let's use a more better name than array. Maybe fruit or favorite movies can inspire you with a better name? Also applies to array_shift.
The docs are wrong here.
Can we use a dataprovider for this? And I'd like to see us to test the edge case where the array is empty. This can all be done with a data provider pretty easily.
Comment #18
jigariusI switched to a data provider except for the error test case where an exception in expected. I also fixed the documentation (which was caused by copy and paste). Examples are also improved now.
Comment #19
heddnCan you post an interdiff? That will help make the review process easier.
Comment #20
jigariusI am attaching a diff of the changes I made after your previous feedback to ensure you have something. I will figure out what an interdiff is and post it ASAP.
Update: I think what I posted with this comment is actually an interdiff. I had never heard the word before.
Comment #21
heddnHere's a link to the docs on creating an interdiff: https://www.drupal.org/documentation/git/interdiff. I didn't see the docs changes in the ArrayPop class in the interdiff, but I found that they were done.
Just a small nit. I almost don't want to mention it, but it would be nice to add. Otherwise this is looking really good.
If you make the array keys 'index_array', 'associative_array', 'empty_array', it will be self documenting.
i.e.
Comment #22
jigariusI am submitting a diff again for now. I saw the interdiff docs - thanks for that. I think I'll need to find some time to try that out sometime soon.
I have removed the comments and used array keys to define test context. However, I have used spaces instead of underscores in keys cause I think it's mostly for the UI only (which looks better without underscores). Let me know if it's better to use underscores and I'll change. Thanks again.
Update: I think the simpletest UI might have some kind of a problem. It shows this text (note the wrongly positioned parentheses):
Drupal\Tests\migrate_plus\Unit\process\ArrayShiftTest->testArrayShift with data set "indexed array"()
Comment #23
heddnMaybe add a key of 'actual' and 'expected' to these too? That would make it even more super clear. I just now noticed that, sorry I didn't mention it earlier.
And the patch file needs to end in .patch for the testbot to pick it up.
Comment #24
jofitz CreditAttribution: jofitz at ComputerMinds commentedAddressed @heddn's comment in #23.
Also included the interdiff and patch for the work in #22.
Comment #25
jigariusOk. So Joe managed to submit his patch before I could upload mine. Great! Progress = Good.
Comment #26
jigariusSome coding standard fixes.
Comment #27
jigariusComment #28
jigariusComment #29
heddnLooks good. Thanks everyone your contributions.
Comment #31
heddnComment #33
benjifisherI ran into this problem, too. It is convenient to have the new
array_shift
plugin, but you can get the same effect like this:Note that
array_values()
calls its argument by value, not reference. Thus it does not have the problem mentioned in #11 forarray_pop()
andarray_shift()
.