Allow empty 'replace' patterns for MachineName render element

I ran into this as well, trying to use the following machine name element in a form for specifying a URL subdomain, in which I wanted to only allow alphanumeric and hypens (not underscores), and in which invalid characters should be removed entirely (not replaced with anything):

    $form['subdomain'] = [
      '#type' => 'machine_name',
      '#title' => $this->t('Subdomain'),
      '#machine_name' => [
        'exists' => [$this, 'subdomainExists'],
        'label' => $this->t('Subdomain'),
        'source' => ['site_name'],
        'replace_pattern' => '[^a-z0-9-]+',
        'replace' => '',
        'error' => $this->t('The subdomain can only contain letters, numbers, and dashes.'),
      ],
    ];

I was experiencing the same warning. Applying the patch fixes it. Thanks @Grayle!

Please convert the patch to a MR so that tests can run :)

Converted patch by @Grayle to a merge request. Back to "Needs review" for tests to run...

Can the issue summary be updated, should be using the standard issue template.

Also as a feature request probably need some kind of test coverage to make sure it doesn't break in the future.

> as a feature request probably need some kind of test coverage

I actually think this should be a bug report, because the machine_name element type already exists, but causes a warning under certain conditions. Updating the title and category to reflect that...

Still probably needs tests. And issue summary update.

Recommendation for test is to see if there is an existing one you can picky back with hopefully just an additional assertion.

Updated issue summary.

Recommendation for test is to see if there is an existing one you can picky back with hopefully just an additional assertion.

Hmm yea - I wonder if this will be tricky, because essentially we need to test to see if a PHP Warning is thrown. The code does "work", and thus a test of the actual functionality will pass, but it's the PHP Warning we are trying to fix.

I always lean on the side of test coverage, microsoft re-enforced that

But there is policy for small changes not needing test coverage https://www.drupal.org/about/core/policies/core-change-policies/core-gat... → . Just a matter of proving the points are hit to not need them.

I would argue this is a case where a test isn't needed, and may not be possible... but leave that to others to decide.

there is policy for small changes not needing test coverage https://www.drupal.org/about/core/policies/core-change-policies/core-gat... → . Just a matter of proving the points are hit to not need them.

Copied and answered the questions below (to the best of my understanding):

An automated test for a bug fix may not be needed if the following are true and if the majority of answers to the questions below is 'Yes'.

1. The issue has clear ‘Steps to reproduce’ in the Issue Summary. ✔️
2. The fix is 'trivial' with small, easy to understand changes. ✔️

Questions

1. Is the fix is easy to verify by manual testing? YES
2. Is the fix in self-contained/@internal code where we expect minimal interaction with contrib? Examples are plugins, controllers etc. YES
3. Is the fix achieved without adding new, untested, code paths? YES
4. Is an explicit 'regression' test needed? NO
5. Does the issue expose a general lack of test coverage for the specific subsystem? If so, is it better to add generic test coverage for that subsystem in a separate issue? NO
6. If this fix is committed without test coverage but then later regresses, is the impact likely to be minimal or at least no worse than leaving the bug unfixed? NO

I removed the "Needs Tests" tag, but please restore it if you disagree.

Looking at the steps I would assume a test could be written.

Moving to NW because the MR is now almost 1000 commits back and still think test coverage is needed.