I find myself needing this a lot... particularly on canonical csvs and the like. I know another way to handle this would be a custom source plugin, or a hook, but I like to make as many things process plugins as possible. Since Callback only allows functions with a single argument, adding this as a reusable Process Plugin and thought I would place it here to contribute to the community.
This plugin is very useful and I'm willing to see it in migrate plus.
Comment | File | Size | Author |
---|---|---|---|
#38 | migrate_plus-string_replace_process-2830512-38.patch | 5.15 KB | Ada Hernandez |
#38 | interdiff-33-38.txt | 590 bytes | Ada Hernandez |
#18 | migrate_plus-string_replace_process-2830512-18.patch | 5.01 KB | ldavidsp |
#18 | interdiff_6-18.txt | 2.5 KB | ldavidsp |
#17 | migrate_plus-string_replace_process-2830512-17.patch | 10.47 KB | ldavidsp |
Comments
Comment #2
generalredneckI really don't know how to create a test when I'm injecting another service into the plugin, so sorry I wasn't able to get tests for this. However, I did document it really well and it's got some bells and whistles
Comment #3
joachim CreditAttribution: joachim commentedLooks useful!
Just a few nitpicks:
I thought I saw recently in some documentation that there's a convention of a '@' prefix on a value in a migration YML to mean a source field. That would mean this extra property isn't needed.
Typos on this line - replacing, and missing capital letter.
Typo.
Comment #4
heddnsearch_source isn't needed. You can build up dynamic values as pseudo fields and pass them around. As an example, d6/d7_file both do this.
Same feedback here. A replace_source isn't necessary.
Comment #5
Charlotte17 CreditAttribution: Charlotte17 at MTech, LLC commentedHere deleted comment #3-4
Comment #6
Charlotte17 CreditAttribution: Charlotte17 at MTech, LLC commentedUpdate patch#5
Comment #7
heddnI'm really liking this. Very close. Some small nits and we need a unit test to check the case sensitive vs non-case sensitive nature of the transform. See http://cgit.drupalcode.org/migrate_plus/tree/tests/src/Unit/process/Skip... for an example of how to write a unit test.
Nit, extra whitespace.
Let's surround 'search' with single or double quotes to call it out more as a parameter.
Let's surround 'replace' with single or double quotes to call it out more as a parameter.
Nit: we could just return the result of this. No need to assign it to a variable before returning.
Comment #8
edurenye CreditAttribution: edurenye as a volunteer commentedThis is a duplicate of #2754523: Regular expression replace process plugin, I should close this issue but as seems to be more progress here so maybe we might close the other one, not sure, up to you, just telling so we can join efforts.
Comment #9
mikeryanYes, this one should have been closed as a duplicate up front, we need to align/merge the two patches. I'll try to find time to examine the latest patches in both issues.
Comment #10
edurenye CreditAttribution: edurenye at ENDPHASYS Technologies commentedThe class path is wrong: "core/modules/file/src/Plugin/migrate/process/StrReplace.php"
This class must be in: "src/Plugin/migrate/process/StrReplace.php"
Comment #11
osopolarThe documentation of str_replace says:
str_replace would be faster, and in some larger migrations performance could matter, so both plugins may have their right to exist.
Comment #12
ldavidsp CreditAttribution: ldavidsp at MTech, LLC commentedComment #14
ldavidsp CreditAttribution: ldavidsp at MTech, LLC commentedComment #15
ldavidsp CreditAttribution: ldavidsp at MTech, LLC commentedComment #17
ldavidsp CreditAttribution: ldavidsp at MTech, LLC commentedComment #18
ldavidsp CreditAttribution: ldavidsp at MTech, LLC commentedComment #19
heddnFeedback from #7 is addressed. But still needs tests so back to NW.
Comment #20
rodrigoaguileraSame patch as #18 but with the paths relative to migrate plus module instead of core.
Comment #21
joshi.rohit100missing diff and tests
Comment #22
rodrigoaguileraA interdiff for the path of the the files that are affected by the patch will look confusing.
The path was:
diff --git a/core/modules/migrate/src/Plugin/migrate/process/StrReplace.php b/core/modules/migrate/src/Plugin/migrate/process/StrReplace.php
That is the only change I made.
Comment #23
tmountjr CreditAttribution: tmountjr commentedProps for #20 - I had to do that manually as the plugin was considered missing because of namespacing problems.
Comment #24
quironHi all,
I test it and I had not namespacing problems. I had a missing 'case_insensitive' key error.
So I commit a patch solving this, and also adding the possibility to use regex for the 'search' string.
What else is need to release it? IMHO this will be useful for a lot of people!
thanks.
Comment #25
quironSorry, small fixup in the diff metadata and code style.
Comment #26
heddnAdding tests to this is a fairly trivial phpunit task. Tagging for that. See #2923891: Transliterate file names as a recent example on how to add this type of test.
Comment #27
heddnThese variables do not seem used.
I do not see how these examples are possible. The code seems to throw an exception if search or replace is missing.
Can we get an example of when or how this would look?
This does not provide an example of this plugin and is mildly confusing why it exists. Let's remove it.
Can this show an example regex? I'd be curious if we need to include the slashes, etc. A more full example with an actual regex would make that more clear.
Comment #28
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commented#27
Comment #29
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedComment #30
heddnCan we provide sample values for the string were this would match?
Example: This example would replace any instance of 'Foo' or 'foo' or 'FOO' with 'bar' in the values migrated in via the source field 'text'
I think this would fail with an exception since it is missing search.
Can I assume this replace a string of '123' with the value 'bar'? Let's include that in the example.
Comment #31
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commented@heddn thanks, examples added, and #2 is removed, and there's an example -> " To do a simple hardcoded string replace" and Case insensitive searches and regular expressions
Comment #32
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedComment #33
heddnLooking good. Now just need a unit test or two.
Comment #34
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedrenaming bar...
Comment #35
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedneed tests
Comment #36
ao2 CreditAttribution: ao2 as a volunteer commentedThis is pretty useful, thanks.
By looking at the code, the only remark I have is that when both
regex: true
andcase_insensitive: true
are set, thencase_insensitive: true
seems to be silently ignored.Maybe something with PCRE pattern modifiers can be done to cover this case, or at least the user should be warned about the ignored option.
Comment #37
osopolarAs #24 also added also a simple regex support (preg_replace) to this patch, documented in #34:
wouldn't it be nice to be able to actually process the matches before replacing them. For example if the legacy site used the node embed input filter to replace the placholder [[nid:23]] with the rendered node content which may be migrate to Entity Embed and may should look like:
<drupal-entity data-entity-type="node" data-entity-id="1234" ... />
.For that purposes I had opened a separate issue: #2754523: Regular expression replace process plugin, but maybe it should be merged into this one. It should work for Entity Embed, or Media Embed but also for all other use cases where someone needs to manipulate the matches before replacing them. The patch allows to expose the matches to the full stack of process plugins.
Comment #38
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedadding tests and fixing pattern
Comment #39
heddnThis is looking really close and quite nice! Just a couple more areas where I think we can add coverage. See https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writ... for testing exceptions.
Let's add a test case for these edge cases too.
And this as well.
Comment #40
edysmpTesting exceptions and multi-value.
Comment #41
heddnWonderful work here.
Comment #43
heddnComment #44
generalredneck👏
Thanks for making my start a reality...