#Needs release manager review

Also did a small search locally about how hard it would be to remove and most references in our tests seem to be in migrations and uninstall/re-install tests.

Sure no problem.

One thing that came up in the slack discussion but not explicitly here, was that https://www.drupal.org/project/telephone_validation → has about 13,000 installs, which suggests that core shipping the telephone module isn't preventing people from having to install contrib modules to actually use it. This is unlike a lot of field types that ship with core and 'just work'.

Stephen would you be prepared to maintain the contrib version?

I think its not an 80% use case module. The general consensus earlier was that it is little maintenance but looks like its not quite true. Quoting the other issue:

One of the biggest challenges with the current Telephone module is that it's just a glorified textfield. It has no option to normalize the telephone number - either for the purpose of validation or formatting.

I don't think its worth doing that investment in core given it is not an 80% use case AFAIS. Removing tag as a product manager.

I bumped this because of 📌 Package Manager actions intermittently blocked on A2 Hosting Active but also now think 📌 Permanently maintain a sandbox directory in package_manager Active is probably more urgent for package_manager/Drupal CMS. If the other issue works, then this issue would be a very nice to have on top (and for the other reasons it would be good), and starting in Drupal 12 seems reasonable, although if we somehow miss Drupal 12 then I'm not sure I'd want to wait until Drupal 13, could be Drupal 12.1 with a PSA since it's still early in the major release cycle.

For me this is fantastic idea which we should definitely do. But given the disruption to patches I'd only do this in a major release so minor updates to sites that have patches applied don't break just because we've removed tests.

There's also the issue that including test changes in the diff increases the likelihood that the patch won't apply, either in the first place because there are conflicts with the older branch already, or if it's not committed anywhere yet, that test conflicts are introduced in minor versions even when the runtime code change itself would continue to apply. So there are benefits to removing test files already (I often do it if I'm doing a local backport of a core MR diff already partly for this reason).

I'm assuming a stance that if people want to use patches of unsupported (non-released) changes in their own project they can be expected to make their own patches. In that case, it's built in git functionality to to exclude files with git's powerful path matching syntax. That works in diff, log and format-patch to name a few of the commands that use the same underlying features.

As an example for a recent Drupal core commit (using --stat to limit the output to what's relevant for the discussion):

$ git diff --stat 422099995ecfe78f1e29248374fc05cd4a556c1d^!   

core/modules/package_manager/src/ExecutableFinder.php                | 22 ++++++++++++++++++++--
core/modules/package_manager/tests/src/Unit/ExecutableFinderTest.php | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+), 2 deletions(-)

$ git diff --stat 422099995ecfe78f1e29248374fc05cd4a556c1d^! -- ':!**/tests/**'
core/modules/package_manager/src/ExecutableFinder.php | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)

Notice how the second diff doesn't include the test files and could be used to create a patch that would work.

The window isn't closed, we deprecate modules up to the last minute because it's not disruptive from an API point of view (and it's a lot of work that's easier to do closer to a major release). We do need a product decision here though.

> Composer patches was brought up, but now that we're using MR diffs on d.o everywhere, it's not recommended to link to the online MR diff anyway for security reasons, instead people should commit a local copy of the patch (or use one uploaded to the issue in some cases). When preparing that patch, it's fine to delete any changes to test files.

That's still going to be fairly fiddly to do.

You're either manually poking around in a .patch file removing bits.

Or you need the git skills to revert portions of a branch, and that's fiddly too -- something like:

> git checkout 11.x -- */test/* [maybe? will wildcards work here?]

and then commit the result to your own local branch. But then if the original MR is updated, you have to repeat that process all over again.

I mean, sure, I often have local branches in my clone of core for backporting MRs to a stable version. But I'm not sure we can expect everybody to do that.

The best bet would be a preprocessor to composer-patches, as has been suggested, but I don't know how close a 2.0 release is of that.

Wanted to bump this again about potentially removing in D12 if that window isn’t closed?

Made a suggested text.

Automatically closed - issue fixed for 2 weeks with no activity.