String Replace Process plugin

Looks useful!

Just a few nitpicks:

1. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,178 @@
   + *     search_source: search_field
   

   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.

2. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,178 @@
   + * You can provide a source field for replaceing as well. using the
   

   Typos on this line - replacing, and missing capital letter.

3. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,178 @@
   + * the search and replace configurations will override what ever is provided in
   

   Typo.

1. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,178 @@
   + *     search: foo
   ...
   + *     search_source: search_field
   ...
   + *     search: "<author>"
   ...
   + *     search_source: @version_api_search_source
   ...
   + *     search: foo
   

   search_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.

2. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,178 @@
   + *     replace: bar
   ...
   + *     replace: bar
   ...
   + *     replace_source: author_name
   

   Same feedback here. A replace_source isn't necessary.

Here deleted comment #3-4

Update patch#5

I'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.

1. +++ b/core/modules/file/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,167 @@
   +  ¶
   ...
   +  ¶
   ...
   +  ¶
   ...
   +  ¶
   ...
   +  ¶
   ...
   +  ¶
   ...
   +  ¶
   ...
   +  ¶
   

   Nit, extra whitespace.

2. +++ b/core/modules/file/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,167 @@
   +      throw new MigrateException('search must be configured.');
   

   Let's surround 'search' with single or double quotes to call it out more as a parameter.

3. +++ b/core/modules/file/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,167 @@
   +      throw new MigrateException('replace must be configured.');
   

   Let's surround 'replace' with single or double quotes to call it out more as a parameter.

4. +++ b/core/modules/file/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,167 @@
   +    $result = $function($this->configuration['search'], $this->configuration['replace'], $value);
   

   Nit: we could just return the result of this. No need to assign it to a variable before returning.

This 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.

Yes, 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.

The class path is wrong: "core/modules/file/src/Plugin/migrate/process/StrReplace.php"
This class must be in: "src/Plugin/migrate/process/StrReplace.php"

The documentation of str_replace says:

If you don't need fancy replacing rules (like regular expressions), you should always use this function [str_replace] instead of preg_replace().

str_replace would be faster, and in some larger migrations performance could matter, so both plugins may have their right to exist.

Feedback from #7 is addressed. But still needs tests so back to NW.

Same patch as #18 but with the paths relative to migrate plus module instead of core.

missing diff and tests

A 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.

Props for #20 - I had to do that manually as the plugin was considered missing because of namespacing problems.

Hi 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.

Sorry, small fixup in the diff metadata and code style.

Adding 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.

1. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,184 @@
   +  protected $migration;
   ...
   +  protected $processPluginManager;
   

   These variables do not seem used.

2. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,184 @@
   + * This example would replace any instance of 'foo' with 'bar' in the values
   + * migrated in via the source field 'text'
   ...
   + * You can provide a source field for replacing as well. Using the
   + * 'replace_source' configuration.
   

   I do not see how these examples are possible. The code seems to throw an exception if search or replace is missing.

3. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,184 @@
   + * You can use both search_source and replace_source at the same time. However,
   + * the Search and Replace configurations will override what ever is provided in
   

   Can we get an example of when or how this would look?

4. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,184 @@
   + *   version_api_search_source:
   + *     plugin: concat
   + *     source:
   + *       - api
   + *       - constants/dash
   

   This does not provide an example of this plugin and is mildly confusing why it exists. Let's remove it.

5. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,184 @@
   + *     search: my_regex
   

   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.

#27

1. variables removed
2. removed because don't match the code with examples
3. ^
4. removed
5. example added

1. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,102 @@
   + * migrated in via the source field 'text'
   ...
   + * Case insensitive searches can be achieved using the following:
   

   Can 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'

2. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,102 @@
   + *   field_text:
   + *     plugin: str_replace
   + *     source: text
   + *     replace: bar
   

   I think this would fail with an exception since it is missing search.

3. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,102 @@
   + *     regex: true
   + *     source: text
   + *     search: /\d/
   + *     replace: bar
   

   Can I assume this replace a string of '123' with the value 'bar'? Let's include that in the example.

@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

Looking good. Now just need a unit test or two.

renaming bar...

need tests

This is pretty useful, thanks.

By looking at the code, the only remark I have is that when both regex: true and case_insensitive: true are set, then case_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.

As #24 also added also a simple regex support (preg_replace) to this patch, documented in #34:

* If the value of text is "vero eos et 123 accusam et justo 123 duo"
* in source, foo is "/\d/" in search and bar is "the" in replace,
* field_text will be "vero eos et the accusam et justo the duo".

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.

adding tests and fixing pattern

This 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.

1. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,104 @@
   +    if (!isset($this->configuration['search'])) {
   ...
   +    if (!isset($this->configuration['replace'])) {
   

   Let's add a test case for these edge cases too.

2. +++ b/src/Plugin/migrate/process/StrReplace.php
   @@ -0,0 +1,104 @@
   +    $this->multiple = is_array($value);
   

   And this as well.

Testing exceptions and multi-value.

Wonderful work here.