Allow dynamic application of patches to dependencies

Well, I think we're still going to have similar legal and security concerns on external patches as we have on external libraries or code. So, I believe we're still going to need some rules on where the patches can live.

When I talked to Dries about this, I suggested one possible approach (which alex_b and I had explored) where we only apply patches to d.o-hosted distributions if the patch file itself lives on d.o. So, if you want to patch something in your distribution, you have to create an issue for that patch, attach your patch to the issue, and then you can use that file URL in your drupal-org.make file. This way, there's always a public record of the patch, a place to document what the patch does and why it's needed, and the enforcement rules inside our drush make plugin are pretty easy to write.

Other suggestions are welcome.

See also #779998: Add support to record patches applied to release packages

I'm pretty much -1 on that one. We are all on d.o here, which is an environment which supposedly promotes cooperation. Allowing patches clearly sends the wrong message.

(This said, couldn't we allow specifying a -dev version, potentially targeted by date?)

@DamZ:

Re: #3: the example use-case is simpletest. I maintain the drupal.org testing distribution. We now have tests for Project*. The distro includes simpletest. However, to use D6 simpletest, you need to patch core. So, behold the craptastic text at the bottom of our latest release:

Simpletest is now included, but you'll need to run this command from your doc root once you unpack the tarball:
patch -p0 < sites/all/modules/simpletest/D6-core-simpletest.patch
If you do that, you'll be able to enable the simpletest module and run tests, including for Project*.

This sucks. I'd much rather just be able to have the packaging script patch core and be able to enable the module in the installer itself.

We should encourage distro maintainers not to patch the code that they use, but there are cases where there's no alternative to patching, and we should support them.

Re: #4: Technically we could allow -dev, but it makes reproducibility a much much much harder problem. I'd rather encourage official releases from contrib maintainers than allow distro maintainers to target -dev.

Could we enforce that the patches are in the issue queue of the target project (I mean: a patch to Views is attached to an issue in the Views issue queue), or would that be too much an hassle?

@DamZ: we probably *could*, but that seems like a lot of extra hassle for only minimal benefit. And people can reclassify issues, which would then break packaging, etc. I think if we say it's in a d.o issue, we're happy (enough). KISS.

Ok, fair enough. What is required to move this forward?

Re what's needed:

A) Agreement from the infra team that we can allow this. I'm 98% sure I got Dries to agree, so I think this is basically done.

B) #779998: Add support to record patches applied to release packages

C) A patch for the drush make --drupal-org plugin to allow patching, but only if the URL begins with http://drupal.org/files/issues or something.

We could either turn this issue into (C) and move it into the drush make queue, or open a new issue in drush make's queue and link it here.

Moving this issue to the Drush Make queue. Let's move forward with a patch that:

(1) allow the 'patch' command,
(2) normalize patch URLs
(3) checks that all patch URLs starts with "http://drupal.org/files/issues" after normalization

One issue we will probably bump into is that we disallow the 'drupal' project, which might make it impossible to apply core patches.

plan in #9 looks good. i'm in strong agreement w/ the following two things:

1. no -dev releases. IMO official releases plus officially used patches are more clear and easier to support
2. patches can only be drawn from somewhere in the d.o issue queue -- this way we can trust their permanence

re: core patches -- since d.o has it's own standardized .make files, i think we can just use a d.o specific attribute in the d.o .make file for this. core_patch[] = /path/to/patch would probably be fine.

Just for good measure: How many people have told you this is overengineered?

Applying to an installation profile which in turn collects modules from d.o? Please.

@killes: I have no idea WTF you're talking about. You don't seem to understand what's being discussed here. Have you actually read the issue in full? You're adding noise, not signal. ;)

what about having the drush_make_d_o plugin disallow any URL's at all in the patch attribute, and having drush_make's default behavior be 'look for the patch at http://drupal.org/files/issues' if just a filename is specified?

this seems cleaner to me. that location is a sensible default, it makes .make files a bit more readable, and it makes this system more portable if by chance we happen to change the default location of our patch files.

Disagree on #14. Having absolute URLs makes everything more portable. There is no reason here to diverge from the standard .make files directives.

Agreed on #16. See also #779998-2: Add support to record patches applied to release packages for a similar argument I just made on another aspect of this effort.

My concern: once you have patched a module, you basically fork the module. This can be insignificant in that the patch gets into a released version and the module is then upgradable, or in some cases you could render the module useless to upgrade.

For instance, someone creates a distribution that uses a patch that has an issue but is still being discussed. Someone uses that distribution. That patch does not make it into the module for whatever reason, and now you have a forked module that may not allow for that user to upgrade that module successfully.

Personally, I think SimpleTest may be the only use case to allow patches into a distribution. And that is specifically because it is patching core which has a much longer release cycle than contributed modules. Any patch that is needed can get into a contributed module reasonably quickly; I don't think its worth the possibly negative implications (described above) over waiting a few days to get a patch into a dev version.

But, to actually be able to address something like the SimpleTest case, I would suggest you only allow patches on drupal.org that are part of an issue that has been marked as fixed. This ensures that at least the module maintainers support the patch. Though there are still workarounds to getting undiscussed, possibly harmful patches into a distribution. Or, maybe a whitelist system (like third-party code).

#779998: Add support to record patches applied to release packages
This issue creates a system where things become more accountable, but there are many users who are not going to understand what a patch is, let alone what it does and why they can no longer upgrade their site successfully. And it is probably arguable that this is the audience for most distributions.

This whole argument could be semi-ignored if you think the responsibility of upgrading is on the maintainer of the distribution, not the individual module maintainers. Then it would be the distribution that would have to deal with a forked module.

Overall, I see the added convenience of allowing patches, but I see the possible pitfalls being much greater than the practical value. The focus of distribution creators should be more on ensuring those patches are stable and that they get into the modules.

My two cents.

I do see your point and it does allow for something like Open Atrium to push the curve, and I think you and your colleagues are responsible developers, but should this be hosted on drupal.org then?

For instance, your work on making Views translatable; I recently read up on this issue and it is still in flux, so what happens if all the people that have translated Views on Open Atrium sites try to upgrade later and the actual patch that gets into Views utilizes a different data structure? They could be totally screwed.

I don't have a big stake in this at all, and as a developer, I think its great to put patches into distributions. But there are cases where its not always good to have things hosted on drupal.org and I think this is one of them. If a distribution needs to use a patch (outside of a case like Simpletest), I think those distributions should be hosted off drupal.org, like Open Atrium and Managing News is currently doing. Just because patches are tracked in a an issue doesn't mean that the patch gets committed or any specific version of the patch is close to what gets committed.

Again, I do really appreciate the direction of distributions and especially Open Atrium and Managing News which are awesome. I just don't think allowing arbitrary patches (even from d.o) is a good thing for distributions that are hosted on d.o.

Let's get some movement on this.

Comments #18 and #20 were not helpful in light of the consensus (including the thumbs up from Dries) on the need for patches in drupal.org distributions (see #9). The examples given (see simpletest in #5) are sufficient and appropriate instances of "non-forking" patches.

Consider this back to #17.

Subscribe

This should do it, though untested.

Tested, needed a couple small typo fixes.

Thanks (w00t!)

When is this functionality expected to go live on drupal.org? It's a little confusing that this is status:fixed but not running on drupal.org.

This is broken thanks to #1282836: Stop trying to move issue attachments to a subdirectory of files. Trivial patch attacahed.

And yeah, this needs to be deployed before this should be marked closed. That will be coming soon, stay tuned.

Here's the same change rerolled now that the drupal.org-specific drush make stuff is moving to http://drupal.org/project/drupalorg_drush (see also #1365572: Remove drupal.org-specific code).

[ Powered by #1115636: Issue Macros and Templates - Drush Make]

Drush make is moving into drush core (discussed in issue: #1310130: Put drush make in drush core)
This means that the issue queue is also moving.

The drupalorg specific stuff is also moving into it's own project, http://drupal.org/project/drupalorg_drush

the patch in #29 is straightforward enough, and should work fine.

one minor concern i have is that the new path opens up the possibility that patches could come from any drupal.org node type that has uploads enabled, not just issues. at the moment, the following additional node types allow attachments: book, changenotice, forum, packaging-whitelist, page, story.

talking w/ killes, there doesn't seem to be a real security concern about this, but it would be nice if we could intelligently filter such that only patches attached to project issue nodes are allowed.

so, before going forward, do we care about this inconsistency enough to look at smarter ways to determine if the patch is attached to an issue?

I don't think it's worth the hassle. I believe we can trust distro maintainers. They don't want to patch the stuff in their distro. If they have to, they're going to be okay with following the recommendation that the patch should be attached to an issue. Worst case, they create a book page describing all the patches they needed to make, attach the patches there, and reference those. Big deal. The point is that the patch lives somewhere on d.o (so everyone can see what it's doing), ideally with some explanation/justification. That's all we really care about.

verified #29 passes tests, committed.

Normally, I'd tag this with "needs drupal.org deployment" and that whole thing, but in this case, it's just one piece in a bigger puzzle. For folks following this issue wondering when it's going to go live, all I can say is you should keep your eyes on http://drupal.org/community-initiatives/drupalorg/distribution-packaging... and when all that's green, this will be deployed. ;) There will also be plenty of announcements about this, perhaps on the d.o front page, but certainly at http://groups.drupal.org/distribution-profiles and in the Drupal Planet.