- 🇬🇧United Kingdom catch
Proposal #1 seems good to me - it's completely reasonable to require people running tests to use prefer-source.
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. This also makes it more likely that the patch will apply longer if unrelated changes to test coverage are committed to the project too, sometimes I do it anyway for that reason, especially backporting something for a previous minor/major release.
- 🇬🇧United Kingdom joachim
> 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.
- 🇳🇱Netherlands kingdutch
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
andformat-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.
- 🇬🇧United Kingdom catch
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).
- 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- 🇬🇧United Kingdom catch
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.
- First commit to issue fork.