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.
Problem/Motivation
It would be nice to be able to call functions which requires more than just one parameter. For example, ltrim($str, $character_mask)
.
Proposed resolution
Add the optional parameter unpack_source
to the callback
plugin. When set, the source value passed to the plugin must be an array, and it is interpreted as a list of arguments. This can be implemented by using call_user_func_array()
instead of call_user_func()
.
Here is a complete example using the new option:
id: callback_test
label: Test the callback process plugin.
source:
plugin: embedded_data
data_rows:
- id: 1
url: https://www.example.com/
- id: 2
url: https://www.drupal.org
ids:
id:
type: integer
constants:
slash: /
destination:
plugin: entity:taxonomy_term
default_bundle: tags
process:
tid: id
name:
- plugin: callback
callable: rtrim
unpack_source: true
source:
- url
- constants/slash
- plugin: log
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
None
Comment | File | Size | Author |
---|---|---|---|
#75 | 2882276-75.patch | 5.29 KB | benjifisher |
#75 | interdiff-2882276-72-75.txt | 1.37 KB | benjifisher |
#72 | 2882276-72.patch | 5.19 KB | benjifisher |
#72 | interdiff-2882276-68-72.txt | 2.43 KB | benjifisher |
#68 | 2882276-68.patch | 5.47 KB | benjifisher |
Comments
Comment #2
osopolarComment #3
heddnThis should extend the core callback method. I think.
Let's pass these as config options to the plugin. Rather than source values.
And needs tests => Needs work.
Comment #4
osopolarI thought about that, but could not see any advantage nor anything in common, besides they do something very similar. And passing the parameters by config make them even more different.
I'll try to fix the other points.
Comment #5
osopolarHow should it look like? Just renaming source to parameters?
Should the $value parameter passed to
CallbackPlus::transform()
method be ignored? I am not sure how this plugin should behave in the pipe. I thought about prepending $value to the parameters array before passing it tocall_user_func_array($this->configuration['callable'], $parameters)
. But it's not always desirable to have value be the first parameter of the called function. For example in functions likeexplode ($delimiter , $string, $limit)
it would be desirable to pass value as second parameter ($string). Maybe there could be a placeholder in the config option parameters.Edit:
Is the naming of the plugin ok or are there any suggestions?
Comment #6
nuezI would agree, however, we cannot reuse anything from the original Callback class. I don't know what's best practice in that case. I've extended the original Callback class anyway.
Done.
Comment #7
nuezComment #9
nuezIt turns out that validation of migration processes takes place in the construct of the class, as of 8.6.x. That means that we can actually extend the Callback class: we would reuse part of the construct.
Comment #10
nuezComment #11
heddnMaybe this shouldn't be called callback then, if we are using 'call_user_func_array'.
!isset == empty, no? At least in this context. Maybe throw some tests at this to prove it.
Comment #12
joshi.rohit100I am wondering about the case where subject/source position is not last.
Example
1. ltrim(), here we need the subject as first argument and this will work with the patch.
2. Str_replace(), here subject should be the last argument and this will not work with current patch.
Comment #13
kristiaanvandeneyndeThis addresses the feedback from #11 and #12 by changing the checks, renaming it to match the original plugin it's extending and by allowing the source to be used at any index of callback parameters. Will try this on my project now.
Comment #14
kristiaanvandeneyndeHmm, wrong use of array_splice and I forgot to map the provided parameters.
Comment #15
kristiaanvandeneyndeWorks like a charm on my end.
Some example usage for migration commerce promotions:
Comment #16
heddnFirst, we should add some tests here.
Can we call this just
index
?I think we need some tests here. If these values are dynamic from the source plugin instead of just static values, then I don't think they will get swapped for anything dynamic.
Maybe call it
call_user_func_array
?Comment #17
kristiaanvandeneyndeRe #16:
call_user_func
, so I figured we should call the representation ofcall_user_func_array
CallbackArrayBut yeah, needs tests.
Comment #18
heddn17.1: We already use index in other core process plugins, so using index here would carry on with that tradition. See Extract as an example.
17.2: Great. But there's a method on the Row as of 8.7 that encapsulates all that same logic for you. See https://www.drupal.org/node/3001535. In which case, we'll want to add a TODO and open an issue to remove the extra code once 8.6 is sunset. And we still want tests on that feature.
17.3: sure enough, it does. Shall we go back to calling it
callback_plus
? Using the name of this module as part of the feature set?Comment #19
kristiaanvandeneyndeAgreed on all 3 points. Also did not know about the issue you linked, that's really useful information.
Comment #20
hudriI came accross this and in search for a wrapper to
call_user_func
. I looked through all the patches, and all the efforts to differentiate betweensource
andparameters
are really confusing. I'm not the expert for Migrate, but astransform()
doesn't use$value
by reference, it technically shouldn't matter if we pass additional (call_user_)function parameters also in the source array?OTOH, by separating source and parameters, the syntax for this plugin has become confusing, the code has become longer, and in the end it does the same as the oneliner in #2 (ok, it's a 5-liner).
What's the reason to separate between
source
andparameters
before stuffing them both intocall_user_func_array()
? And is this reason really worth all the additional code? I'd stick to KISS and go back to #2Comment #21
estoyausenteHi all,
thanks for the effort applied in this code, it's really useful for me. I just developed a similar code to resolve my own problem when I found it. So I merged both (my code and this patch) and now I think that it's more stable.
Following the last comments:
I changed the var to use index in order to follow the traditional name in other parts of migrate and migrate+ as suggested.
And it works perfect! I used it changing the original code.
I'm not sure about it, CallbackArray seems as good name as the other, you know that one of the most difficult thing in development is set names... ^^ I maintain for the moment CallbackArray. :)
And the last and important point, I added some Unit tests for this plugin.
Please, I'm happy if someone can review it.
Comment #22
estoyausenteComment #23
estoyausenteHi again,
A college discovered that the code doesn't work with constant. Some of the functions use constant to work and be configured (for example the function array_unique) and it's very useful allow do use it. I know that you can use the constant original value in place of the constant token, but it's not very intuitive and it's difficult to read the code.
I added the possibility of use constant in the definition, but with an extra config param allow_constant that have to be defined to make the replacement. I create the extra param because I think that is the best way to avoid problems with replacements not wanted.
I added 2 more unit tests case to manage this new param.
Comment #24
benjifisherThe tests are not running. For example, looking at the testbot results for the patch in #23, I see
We need the tests to run (and pass), so I am setting the status to NW.
Comment #25
estoyausenteSorry, I forgot to add the @group annotation.
Added all annotation to the test,
Comment #26
michel.g CreditAttribution: michel.g at Randstad Digital for Government of Flanders commentedI just reran those tests, which had the same failures as the parent branches
Comment #27
benjifisherFrom the issue summary:
Actually, I think it would be pretty easy! The trick is to use the "splat" operator, which turns an array into an argument list.
The implementation of the
callback
plugin is so simple:All we have to do is add a configuration option; for now, I am calling it "splat":
Then the example from #2 becomes
The examples in #15 are trickier. I think the first one could be done like this:
My version is one more line, but I think it is easier to see what it is doing.
The second example from #15 becomes
Caveat: I have not tested any of these examples.
Comment #28
benjifisherOn second thought,
!empty(...)
is more familiar than... ?? FALSE
.For implementing the "splat" option, we could be shorter but perhaps less clear:
Or we could save a couple of lines in the original suggestion:
Comment #29
benjifisherWe discussed my suggestion from #27-28 at #3200725: [meeting] Migrate Meeting 2021-03-04 and the feedback was very positive, so I am moving this issue to the core queue and supplying a patch. I am not attaching an interdiff, since this is an entirely different approach.
A few changes from my original suggestion:
call_user_func_array()
if the new option is set.array_args
The patch includes test coverage.
I have not yet updated the issue summary, so I will add the tag for an update.
The class comment used to start off,
I changed that to
Perhaps I should change it a little more.
Comment #30
benjifisherSorry, I attached a bad patch to the previous comment.
Comment #31
benjifisherDisable spell check for one line.
Comment #32
danflanagan8Should this key be marked as optional here? Looks like
array_args: (optional) Whether to interpret the source as an array of arguments.
would be in line with other documentation.Comment #33
danflanagan8I personally think we could come up with a better example here. This specific case could easily be handled with the format_date plugin, right?
Is there a use case we could present that can not be accomplished with another core plugin?
One of the tests added for this new feature uses
rtrim
as the callable function. Is there a good example withrtrim
as the callable?Comment #34
benjifisher@danflanagan8:
Thanks for taking a look!
#32: Yes, I will add "(optional)" in the next patch.
What do you think about the code comment (#29)?
#33: What do you think of replacing both the
rtrim
test and the example in the doc block with something like this? (needs testing)Comment #35
danflanagan8@benjifisher,
I love the new rtrim example. That's really easy to follow and totally practical. If your updating tests, I would be interested in seeing a test where the function takes three parameters just so there's some more variety in the tests. I know there's an
str_replace
plugin in migrate_plus, but I seestr_replace
as a good callback with three arguments to test.Regarding the comment in #29, I don't have a big problem with it. If I were forced to make a change, perhaps I wouldn't mention "creating" an array because that might make it seem more complicated than it is. I would describe "passing" an array.
As long as
array_args
is set I think the array is interpreted as args whether it's something like this:or
In the first case, maybe you already have the array and nothing needs to get created. In the second case, I don't really feel like I'm "creating" an array. "Creating" makes me feel like it should be a separate step or something. But maybe I'm reading too much into the word create.
Comment #36
benjifisherThe attached patch implements the changes discussed in #32-35.
As a bonus, the example using
rtrim()
does not have a string that will make the spell checker unhappy.I still have to update the issue summary.
Comment #37
benjifisherComment #38
MatroskeenI didn't review the code in detail, but quickly checked the latest patch and noticed we already have test coverage and examples 💪
I also like the idea, and I could probably write fewer custom plugins if I had this feature before.
This is gonna be a new feature, which means we need to create a change record. (moving to Needs Work for that)
I'm also wondering if it makes sense to create a follow-up to deprecate existing process plugins in favor of
Callback
.Comment #39
danflanagan8#36 looks really good to me.
Comment #40
benjifisher@danflanagan8:
Thanks for the encouragement!
@Matroskeen:
Thanks for the nudge to write a change record. I borrowed a couple of examples from the documentation block: here is the draft change record.
Comment #41
Matroskeen@benjifisher thanks, CR looks good to me. I'll let someone else review and, hopefully, RTBC.
Comment #42
danflanagan8This works great for me. I was able to replace a usage of the `str_replace` plugin from `migrate_plus` with the config structured like the sample below:
The migration I ran had other instances of the `callback` plugin with single argument functions. None of that broke or required any changes.
Setting as RTBC.
Comment #44
benjifisherI just added #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest for the unrelated test failure. Let's see if there is any response on that issue before we ask the testbot to reconsider.
If the testbot just switched to DST, then a lot of tests will fail, and we do not want to queue them all up at once.
Comment #45
Spokje#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Ordered retest and put this issue back to RTBC per #42.
Comment #46
SpokjeComment #47
alexpottI really like the solution here - using the existing callback process plugin is very elegant. The think that's got me pondering is whether the option
array_args
is well named. I'm bit fearful of painting a bikeshed but at first glance this is a bit ambiguous. I think what we're really doing here is unpacking the source. I made the change locally and to me it reads better because it reflects that this is about the source and reflects what happens to it. Also the language of unpacking is directly from PHP - see https://3v4l.org/khmSRComment #48
danflanagan8As usual, naming things is the hardest part! While I see that "unpack" is used in this situation with arrays, I also see that there's a completely unrelated built-in php function called unpack: https://www.php.net/manual/en/function.unpack.php. I wonder if that would cause any confusion.
I think @benjifisher originally used the term "splat" which exists in other languages. I'm not seeing it when I search the php manual.
I'm going to throw out an idea that I don't necessarily like, but occurred to me at one point. The new boolean determines whether the plugin should leverage
call_user_func
orcall_user_func_array
. It would make a lot of sense to call the new booleancall_user_func_array
such thatcall_user_func_array: true
would do the new thing with multiple args.The reason I don't like that is that it's not abstract in any way. If for some reason the underlying code changed, then that parameter might get confusing. Or maybe the plugin is so strongly tied to
call_user_func
andcall_user_func_array
that there's no real need to abstract anything.Comment #49
alexpott@danflanagan8 in the implementation in #47 call_user_func is used in both instances. And unpacking is what PHP calls it... see https://www.php.net/manual/en/migration56.new-features.php#migration56.n... and https://3v4l.org/khmSR ... obvs it is amusing the PHP anchor uses splat - but I think we should go with docs.
Comment #50
benjifisherI first came across the term "splat" in a StackOverflow answer. Looking it up, I found the same page mentioned in #49:
I originally thought of implementing this feature with
call_user_func(...$value)
. I used "splat" in some comments here as a working name for the new option, but never thought it was actually a good name. I decided to use the older implementation,call_user_func_array($value)
.I am happy to use something other than
array_args
. I will ask for comments on Slack.Comment #51
danflanagan8@alexpott I totally missed the change from using
call_user_func_array
to usingcall_user_func
with...
. I retract my comment!Comment #52
benjifisher@danflanagan8, I think you have that backwards. Was I ambiguous?
Comment #53
danflanagan8@benjifisher I was referring to @alexpott making the change away from
call_user_func_array
to unpacking in the patch in #47, not to the opposite change that you made in #29. So we're back to where you started, I guess!Comment #54
quietone CreditAttribution: quietone as a volunteer commentedbenjifisher asked in #migration about the name of the new property which is
unpack_source
in the patch in #47. Bothunpack
andunpack_source
work for me. I like the brevity ofunpack
and in context I can't think of what else it would refer to other than the source but then I am used to mucking around with process plugins. The extra clarity ofunpack_source
is likely to help others and that basis I lean tounpack_source
.Does not need to be quoted.
Comment #55
MatroskeenSince
array_args
doesn’t hold any source and just a boolean that indicates whether to use “unpacking” or not, I would vote forunpack_source
option.Comment #56
marvil07 CreditAttribution: marvil07 at Adapt commentedThanks for making
callback
process plugin more useful!On the option name, both something simple as
single_argument
or the suggestedunpack_source
sound good.One suggestion, I am wondering why we need the two cases.
i.e. creating an array of one element if the option is unset, and always call it the same way.
I am guessing that not enforcing the array is about being back-wards compatible, so I guess the option is needed for that differentiation.
Not sure about the process, but we may want to just change the input to be required as an array in the future.
Comment #57
alexpott@marvil07 imagine the callback was array_unique and you wanted it to filter an array of items - it that instance automatic unpacking would break. That's another about
unpack_source
- it is very explicit about what is going to occur when the callback is processed.Comment #58
marvil07 CreditAttribution: marvil07 at Adapt commented@alexpott, indeed, I forgot about array operations, thanks for pointing that out!
That means the option is truly needed.
Applying the patch from #47 failed, likely b/c of the unrelated/duplicated hunks on
core/modules/field/tests/src/Functional/FieldHelpTest.php
.Looking at git history, it seems like it is colliding with the change from commit
45e5a8aedb0c73beb7f44f96f5bc9647ba1d4a95
.Hence, I am marking as NW.
I have tested the changes locally with a simple migration on a custom module, attaching it here for reference.
The changes work as expected!
Also, please notice that the last patch changes the option to be
unpack_source
which seems to be the consensus.I would RTBC, but that is pending on a re-roll to remove the mentioned two conflicting hunks, to be removed, so NW.
Comment #59
benjifisherI will update the patch now that we have settled on a name for the option.
Comment #60
benjifisher@marvil07:
The patch in #47 included an unrelated change to
core/modules/field/tests/src/Functional/FieldHelpTest.php
. It is a bit of relief to see that I am not the only one who occasionally makes a goof like that.The attached patch makes the same changes as #47 (replacing
array_args
withunpack_source
) without the accidental change toFieldHelpTest.php
. I also removed the unnecessary single quotes (#54).I made corresponding changes to the issue summary. I tested locally with
callback_test
as in the issue summary.I will also update the change record.
Comment #61
alexpott@benjifisher thanks for fixing my goof - definitely not my first time :)
Comment #62
alexpottCrediting everyone who's contributions to the issue have affected the current patch.
Comment #63
quietone CreditAttribution: quietone as a volunteer commentedEvery time I look at this I want it to be something else. I know the ellipsis is correct but can it be 'foo'?
Process plugins should throw a MigrateException not an InvalidArgumentException. We want to control the processing in MigrateExecutable and record messages etc etc. Referring to Extract for an example, let's use this.
I read the test and it does test the new functionality with a likely popular and simplistic case. Does it need more complex cases as well. I am not sure.
Comment #64
danflanagan8@quieone, I had the same though about adding a test with more and/or more complicated arguments. I wonder what we would think about something like this as an additional line in the data provider:
'str_replace' => ['str_replace', [['one', 'two'], ['1', '2'], 'One, two, three!'], '1, 2, three!'],
It is different from the others in that 1) the function uses three arguments and 2) the arguments are a mix of arrays and strings. I think both of those would be nice coverage to add.
Comment #65
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedMade changes as per comment #63.
Comment #67
benjifisherI just thought of another use case. We already have the
str_replace
plugin, which is a wrapper forstr_replace()
orpreg_replace()
, but I want to putpreg_match()
in a pipeline withskip_on_empty
.Comment #68
benjifisher@ravi.shankar:
If we change the exception we throw, then we also have to update the test. If you can set up your local environment to run tests, then it is good practice to test any tests that your patch changes before uploading your patch to an issue.
@quietone:
The Callback class already throws InvalidArgumentException in its constructor. So I wonder whether we should fix that in this issue or keep the exception from #60 and change all of them in a follow-up issue.
Looking at other process plugins, I see that MigrateException is used all over the place, including one constructor: MachineName.
I also searched the other process plugins for InvalidArgumentException, and found only one use, where it was being caught, not thrown. Based on that, I am going ahead and updating the constructor here.
While looking through the other process plugins, I noticed that the
@throws
annotation was added inconsistently. Let’s create a follow-up issue for that. Is it appropriate to add it to MigrateProcessInterface, or should we add it to the implementations that explicitly throw an exception?@danflanagan8:
Thanks for the specific suggestion. The nice thing about provider callbacks is that it is so easy to add test cases.
Comment #69
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, Thanks for the changes.
I am pleased to see the additional test. The changes to the test and the change to using a MigrateException in the transform look good to me.
I didn't look at the constructor until now and I agree that it should be changed but if I understand scoping then it should be a follow up. I did a bit of research and found that the patch that added the InvalidArgumentException originally used a MigrateException but heddn requested it be changed in 2936463#27. I think we should ask him about that.
In reviewing the patch I found this concerning the change to MigrateException.
The coding standard examples for exception messages have the property/field in single quotes.
"The 'callable' must be a valid function or method."
. Let''s change them to that format.Now to consider the question about adding @throws to MigrateProcessInterface. I've been dabbling in the coding standards issues recently and fixes are done by sniff, not individual cases or modules. I don't think we should make an issue to fix this.
Actually, on further thought, we can fix the exception in the constructor and the @throws in a single issue. One that is scoped to ensure that core process plugins are throwing MigrateExceptions and to update the docs in \Drupal\migrate\Plugin\MigrateProcessInterface::transform. What do you think?
Comment #70
benjifisher@quietone:
Thanks for putting some thought into this.
One reason I prefer to change the exceptions in the constructor as part of this issue is that we have a single
testCallbackExceptions()
, with a data provider, that tests the exceptions from both the constructor andtransform()
. If we throw MigrateException intransform()
and InvalidArgumentException in the constructor, then we have to make that test a little more complicated. That is not a problem if we really want to have two different exceptions, but if we plan to end up with consistent exceptions then it seems like more churn than it is worth.As I said in #68, I am also willing to throw InvalidArgumentException in
transform()
, and make no changes to the constructor, and then change all of them in a follow-up issue.Comment #71
heddnThe rational for invalid argument is because that is what we have in the constructor. We have an invalid argument. Actually, I prefer invalid plugin exception. Which I think was also added. The object is a Singleton after construction. We don't want to effect flow here. We want to inform the developer they need to fix things. Move things that never change, things that are validation of configuration to constructor. Because you can't change config. So no need to check it in every invocation of the plugin. Just check it once at construction and be done.
Comment #72
benjifisher@heddn, thanks for explaining!
I think the right thing to do is to stay within the scope of this issue. The attached patch reverts the changes to the constructor and makes the test a little more complicated. As long as I am doing that, I think it is in scope to add a doc block to the test.
By "invalid plugin exception" do you mean InvalidPluginDefinitionException? If we want to throw that in the constructor, then we can change it in a separate issue, and consider the MachineName process plugin at the same time.
Comment #74
quietone CreditAttribution: quietone as a volunteer commentedI reviewed that patch and could only find the following small suggestions.
I think it would be easier to read if these were multiline.
$value is not used and can be removed.
Regarding the exceptions, maybe a topic for a meeting? Just thinking out loud.
Comment #75
benjifisher@quietone, thanks for the suggestions. The attached patch addresses both points in #74.
BTW, I came up with another application. My site has a List(text) field with three possible values. My source is a CSV file with a column for each of the three values. The following snippet does it.
Comment #76
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, thank you. Nice example too! That should be included on the 'nifty' process pipelines examples page. Wasn't there a plan to make such a page?
I re-read the change record and added link to the rtrim() documentation as suggested by danflanagan8 in a comment. The IS is correct as well.
Onward to RTBC.
Comment #77
benjifisherThere is already such a page! I added the example in #75 as Example: combine boolean fields into a List(text) field.
Comment #78
benjifisherOops, I did not mean to change the status.
Comment #79
alexpottCommitted and pushed a3ca88ebbf to 9.3.x and 50d267d893 to 9.2.x. Thanks!
Comment #82
quietone CreditAttribution: quietone as a volunteer commented@benjifisher, thanks for the link. I was on the parent page and missed it! The page is now bookmarked.