dom_str_replace should accept empty replacements

Created on 7 August 2023, 11 months ago
Updated 14 March 2024, 3 months ago

Problem/Motivation

I want to get rid of a certain prefix in certain DOM element attributes. This is currently impossible, as DomStrReplace.php throws "Configuration option 'replace' is required." errors when I try to replace the prefix with an empty string.

Steps to reproduce

Configure dom_str_replace as follows in the migration yml:

search: 'my_search'
replace: ' '

Proposed resolution

Allow the 'replace' option to be an empty string, as it seems a valid use case to me.

✨ Feature request
Status

Needs review

Version

6.0

Component

Plugins

Created by

πŸ‡³πŸ‡±Netherlands koosvdkolk

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @koosvdkolk
  • πŸ‡³πŸ‡±Netherlands koosvdkolk
    <?php
    if ($option_name !== 'replace' && empty($this->configuration[$option_name])) {
    ?>

    seems to do the trick.

  • πŸ‡ΊπŸ‡ΈUnited States alison

    I ran into this today, while trying to fix a bunch of alt attribute values that people left with meaningless placeholder values (and they should really be empty anyway).

    This worked, on line 95 -- h/t @danflanagan8 on Slack:

    if (!(array_key_exists($option_name, $this->configuration))) {
    

    But I can also see the merit of @koosvdkolk's suggestion in #2, because (again, h/t @danflanagan8 on Slack), maybe no other options should be allowed to be empty?

    Probably we need maintainer input on this!

  • First commit to issue fork.
  • Status changed to Needs review 3 months ago
  • πŸ‡΅πŸ‡ͺPeru marvil07

    I was also trying to do a replace with empty today, and noticed the problem.

    But I can also see the merit of @koosvdkolk's suggestion in #2, because (again, h/t @danflanagan8 on Slack), maybe no other options should be allowed to be empty?

    Exactly, in all other cases options needs a value, so allowing empty of them may not make sense.

    I ended up with a variation of the same idea in the code above.

    I opened related MR !89 about it, with one commit.

    - 804f2e2 Allow replacing with an empty string on dom_str_replace migrate process plugin

    Marking as NR.

  • Pipeline finished with Success
    3 months ago
    Total: 213s
    #119385
Production build 0.69.0 2024