PATCHES.txt was created as a quick solution to a larger problem. It was introduced in issue #615570: Create a PATCHES.txt file, and was first released in beta 2. It remains unchanged since.

The current format is *extremely*, and probably excessively, simple: it's a list with "- " prepended to every patch name, and at the bottom, it says "This file was autogenerated...".

This is underserving us as I can currently see it. For example, if you inherited a site made with drush make, that had some patches applied, wouldn't you be curious to know what purpose they served? Or, where they came from?

Another thing we can do is using http://drupal.org/project/drush_patch_tools, come up with some sort of smart layered patch undo system: if I run drush patch foo, drush patch bar, and then drush unpatch foo, it shouldn't do a straight -R - it should first patch -R bar, then patch -R foo, then patch bar. (Another thing this could be useful for is making several files on top of each other, not all at the same time).

So there are two things that need addressing in this hypothetical new format: It needs to contain more information, and it needs to be machine-readable (and writable, of course).

This brings up the question of what format to use, and which information to include.

For information, I would propose that the only necesarry piece of information is the file URL, but on top of that, two other optional pieces of information are the source and title.

As to format: we have many choices. The .info format is certainly attractive. But, it's quite verbose, and very machine-y for something that I want to be very human readable (example of what it may look like: http://drupalbin.com/16051). Another idea would be JSON, which I don't like, because it's not very human readable. YAML is certainly attractive, but lacks a good PHP parser (and, I don't want an external library) and contains very many ways to do the same thing (hearing noises blurting "perl" in the background). Thus, I think we have to come up with our own format.

The format I propose is very simple. It consists of entries, each containing several attributes, which are each paired with values. Entries are separated by ===, and a final === is included. That is to say, after every entry, there is a ===. Each attribute-value pair is in the following format: - attribute: value, each on a separate line. Everything after the : is considered the value: the attribute may not contain a colon. Quotation marks are not string identifiers, but are interpreted as literals. An example PATCHES.txt written in this format can be seen at http://drupalbin.com/16052. [EDIT: Please see comment 5 for a clarification and revision. END EDIT]

Lastly, a PATCHES.txt will reside in every directory in which a patch was applied, and will contain only the patches applied directly in that directory. This is the point I'm the least sure about, but I'm not sure how it would work if it were any other way.

So: discuss!

Comments

dmitrig01’s picture

dmitrig01 commented

Also, in the proposed format, on any line, if there is a # sign, anything including and after that is a comment.

hefox’s picture

hefox commented

Came here from twitter; in a PATCHES.txt in a root of a non-drush-make site, we find it useful to have a list of files effected by the path, so just throwing that in.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

As I understand, this is a proposal for PATCHES.txt but nothing is said about any change in makefiles isn't it?

I think a .info format for patches in makefiles is very appropiate. Not very convinced but perhaps something like:

patches[patch-name][title] = "Allow views defaults static cache to be cleared"
patches[patch-name][file] = "http://drupal.org/files/issues/views-static-cache-resets-694094-7.patch"
patches[patch-name][source] = "http://drupal.org/node/694094"

project[views][patch] = patch-name

It could be also handy to support an alternate source format, as almost all patches come from d.o:

patches[patch-name][issue] = #12345: How do I log in as admin? # comment 12 on issue 12345

PATCHES.txt should extend the issue number to the complete url.

my 2cents

irakli’s picture

irakli commented

I think it is important PATCHES.txt to also contain information about unsuccessful patch attempts. Currently the only way to debug an error during patching is to run drush make with --debug and look at actual error messages. Can be pretty troubling for larger installation profiles.

Thanks

dmitrig01’s picture

dmitrig01 commented

This is about not about makefiles.

The idea here is it should be a standardized form in which everyone would be able to maintain their PATCHES.txt, automated or not. The nice thing is that if it's standardized it, it's consistent, and machines can manipulate it too.

The idea here is, for example, that Drush Make should be able to pull the drupal.org title and patch off the issue queue, so you only need to type #12345: How do I log in as admin? (like jonhattan said), and drush make will know the patch URL, issue URL and issue title, and will put all those in the PATCHES.txt.

Another thought donned on me the other day for the format: what about using two newlines to separate entries, instead of a (tedious) ===. For multiline properties, like a description if necessary, subsequent lines must start with two spaces. here's an example: http://drupalbin.com/16109?nocache=1

dmitrig01’s picture

dmitrig01 commented

@irakli, i think that this is a problem that should be solved outside of PATCHES.txt - if the patch fails, it should be reported in the drush make output. It would be great if you could file a new issue.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

@dimitrig01 sometimes one attach several files to an issue. For example alternate solutions for the same problem or a screenshot or ... so it is not reliable to just take an attachment from #12345: How do I log in as admin? and we need both the absolute path to the patch and the issue number.

On the format. This is just enough to be parsed by human or machine:

title =
file =
source =

title =
file =
source =

- at beginning of lines or === as a separator are just decoration. Description should be enclosed in quitation marks if they are able to have newlines.

OTOH. I think PATCHES.txt should reside in the top level directory of the project being patched, not in every subdirectory a patch is applied. (The opposite is what I understand from your comment in #0)

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Some ideas & questions:

  • Should this be handled with a separate file per-module & per-theme, or could one file be used to manage the entire site's patches? I would recommend one file per site to cover the entire core & contrib structure.
  • How about adding an extra attribute named e.g. "for" that indicates what the patch is for. There would then be additional logic behind its handling:
    • If PATCHES.txt is in the site root, items that do not have the "for" attribute are assumed to mean for Drupal core.
    • If PATCHES.txt is in the site root and contains a "for" attribute, the item is assumed to be for that contrib module/theme/etc.
    • If PATCHES.txt is in a module directory the "for" element would be ignored.
febbraro’s picture

febbraro commented

To follow on to what Damien said, I have a real need to know which module/version has certain patches applied. When maintaining distributions it is important for us to know what is happening on a site, if a site maintainer has upgraded a module and blown a key patch away, etc. Having this information in a file that can be processed/queried is incredibly valuable to us distro maintainers.

dmitrig01’s picture

dmitrig01 commented

@jonhattan - good point. Let's use : instead of = to not confuse with .info though.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

I intentionally introduced = instead of :

using .info ini-like syntax is fine for me. we don't need to reinvent the wheel, and the parser is already done ;)
http://api.drupal.org/api/function/drupal_parse_info_file/6

as PATCHES.txt is of multivalued nature, using .info format would imply using something similar to what I posted in #3 for makefiles.

dmitrig01’s picture

dmitrig01 commented

We'd need to re-do, or at least modify, the parser, because using the .info format, there isn't any grouping: if we define two source =, the first is overridden.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented

Haven't tested it but as documentation says, the parser can handle those cases:

key[] = "numeric array"
key[index] = "associative array"
key[index][] = "nested numeric array"
key[index][index] = "nested associative array"

It's also extensively used in themes' .info files. Example from basic theme:

regions[content_top] = Content top
regions[header] = Header
regions[left] = First sidebar
regions[right] = Second sidebar
regions[footer_block] = Footer
regions[content_bottom] = Content bottom

stylesheets[all][] = css/default.css
stylesheets[all][] = css/layout.css
stylesheets[all][] = css/style.css
stylesheets[print][] = css/print.css

Each patch will need a custom key (numeric or not). It can be an advantage for makefiles, as I wrote in #3.
Other approach is to autogenerate it or use the issue number for the grouping key, but it is limited to patches from drupal.org.

patches[12345-67][title] = 
patches[12345-67][file] = 
patches[12345-67][source] =
dmitrig01’s picture

dmitrig01 commented

Yeah, the necessity for a key was the part I wasn't so fond of. Since we can't always have smart keys like that, sometimes it'll have to be numeric. Also, saying patches[] every time is repeptitive - what else would you put in this file besides patches?

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

What if the patch wasn't from d.o but instead it came from a company's internal issue queue and was not allowed to be submitted to d.o?

dmitrig01’s picture

dmitrig01 commented

Well then the file source and titles could all apply, they just couldn't be automatically generated froma [13245-12] format

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented

Subscribing. Really like the idea of this feature!

IMO the info file format is most attractive. dmitrig01 has some points on the negative things with the format. But I still think it makes sense to be consistent. The info file format is well recognized in Drupal.

dmitrig01’s picture

dmitrig01 commented
Priority: Major » Critical

hm, i guess we're still kind of stuck on the format.

dmitrig01’s picture

dmitrig01 commented
Status: Active » Postponed

I'm convinced of .info, if we introduce something like the following format:

patches[] = [title = foo, file = bar, source = baz]

Or something... anyway, we can discuss modifications like this in #911722: Default path for projects, libraries, and I'm going to mark this postponed until then (i.e., aiming for 3.x)

febbraro’s picture

febbraro commented

I think having the patches in the .info file is key b/c then you can be sure which version of the module has the patches applied. If you use a PATCHES.txt it is possible for someone to download a new version of the module (or use drush dl?) and overwrite the module files but leave the PATCHES.txt still in place, inaccurately reflecting the state of the module.

On another slightly off-topic, once this is in place what do you think about trying to get drush dl amended to provide a warning if you are trying to download a module that has patches applied. Something like "There are patches applied to this module that might not be in the release you are downloading. DO you want to continue?" So nothing that prevents you form doing anything, but will certainly warn you that you are likely about to screw up your site.

helmo’s picture

helmo commented
Status: Postponed » Closed (won't fix)

[ 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 Drush project has a component called 'Make' for this purpose.

We would like to take this opportunity to leave behind old/obsolete issues, allowing us to focus on a stable make command in core.

If you feel that this issue is still relevant after the move, feel free to re-open and move it to the Drush queue

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
commented
Project: Drush Make » Drush
Version: 6.x-2.x-dev »
Component: Code » Make
Priority: Critical » Normal
Status: Closed (won't fix) » Active
greg.1.anderson’s picture

greg.1.anderson commented

I agree that the current PATCHES.txt format is a little plain, but it is not too hard to parse: just ignore any line that does NOT begin with "-". This format is already supported by #1366894: In generate-make, find git repository for projects not downloaded from drupal.org.

While I can agree with the motivation behind this issue, I tend to think that the proposed solutions are more complex than necessary. If we leveraged #1078108: Drush issue queue commands, then it would be possible to support things such as:

- #910732
- #1078108-2
- http://drupal.org/node/1078108#comment-4165856

... and of course the current format would also remain a supported form. If we did it this way, we would not obsolete any PATCHES.txt that may be scattered around (perhaps quite widely). I don't think there is any need to put duplicate info from the issue queue in PATCHES.txt. If you want it there for documentation, put it in as comment lines (lines that do not start with "-"). If you want it for automation, use the form "- #1078108-2", and pull the metadata with the drush issue-queue commands.

This approach does require getting the issue-queue commands finished up, but they're pretty close. I think putting efforts there first would be preferable.

greg.1.anderson’s picture

greg.1.anderson commented
Version: » 8.x-6.x-dev
Status: Active » Closed (won't fix)
Issue tags: +Needs migration

This issue was marked closed (won't fix) because Drush has moved to Github.

If desired, you may copy this task to our Github project and then post a link here to the new issue. Please also change the status of this issue to closed (duplicate).

Please ask support questions on Drupal Answers.