I was going to re-launch a test to see if
#961172: All D6 Core patches are failing
was fixed.

So I went to
#612870: Weight fields should be int, not tinyint
where comment #23 has a patch file called
612870-increase-weights-d6.patch

The file is marked "ignored", although the issue is Drupal Core, version 6.x-dev, and status Needs Reviewed.

So I couldn't launch the test bot on it. I think this is a bug in the code that is deciding to ignore patches with -D6 file names, for Drupal 7? Hopefully I've filed this in the correct issue queue...

Comments

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
commented

I experienced the same thing a couple days ago in comment #7 of #335621: PHP strict warning in template_preprocess_block_admin_display_form. Dave Reid told me to just get rid of the -D6 suffix, which indeed queued the subsequent patch for testing... then of course, that patch hit the other testing bug #961172: All D6 Core patches are failing.

boombatower’s picture

boombatower commented
Project: Project issue file test » Drupal.org infrastructure
Version: 6.x-2.x-dev »
Component: Code » Other

We had the restriction in place purposefully due to D6 not being supported originally. This is a setting that simply needs to be changed. The default is already set properly in the code and drupal.org should use the same value.

/(\.diff|\.patch)$/

I believe the pift_description field will also need to be updated as it (at least used to) mention the D6 limitation.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

We still don't want to test patches with the -D6.patch extension using Drupal 7 code (e.g. if someone wants to upload a D6 and D7 patch at the same time to fix an issue present in both). Will that be possible to restrict if the setting is changed? It seems to me that any patch with an extension -Dx.patch should not be tested unless the version of the issue is Drupal x.y?

boombatower’s picture

boombatower commented

Nope, get one or the other. Suppose it makes more sense to leave it since patches in d6 have no reason to require D6 in the name.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

In that case, how about changing it so that any patch with a -Dx in the name doesn't get tested, and changing the help text on the attachment field accordingly? Just a thought -- people seem to want to make patches for more than one version at the same time. [I personally rarely do that, as I usually think it's better to get the patch to RTBC in one version before bothering to make a patch for another version].

Actually, right now the Attachment field doesn't have the note about -D6 any more that I can see (at least not in this issue), so I guess this has been done already?

boombatower’s picture

boombatower commented
Status: Active » Reviewed & tested by the community

The comment only displays in projects that have testing enabled..."Drupal.org infrastructure" does not have testing enabled. I think changing the regex to ignore all Dx sounds like a good idea.

Anyone from infrastructure team change the 6 to \d?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

Or perhaps \d+ (we may get to Drupal 10 some day, though probably not soon if D7 is any indication)...

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

Old value was:

/(?<![.\-_][Dd][56])(\.diff|\.patch)$/

Old description was:

Only files ending in <em>.patch</em> or <em>.diff</em> will be sent for testing. For patches that apply specifically to Drupal 5 or Drupal 6, you can prefix the extension with <em>-D5</em> or <em>-D6</em> to prevent them from being queued for testing. For example, <em>foo.patch</em> and <em>bar.diff</em> would both be queued for testing, whereas <em>foo-D5.patch</em> and <em>bar-D6.diff</em> would not be queued for testing.

New value is:

/(?<![.\-_][Dd]\d+)(\.diff|\.patch)$/

New description is:

Only files ending in <em>.patch</em> or <em>.diff</em> will be sent for testing. For patches that apply specifically to a version of core other than the one the issue is pointing to, you can prefix the extension with <em>-D(number)</em> to prevent them from being queued for testing. For example, <em>foo.patch</em> and <em>bar.diff</em> would both be queued for testing, whereas <em>foo-D5.patch</em> and <em>bar-D6.diff</em> would not be queued for testing.

The new description could probably use some help, so if someone wants to propose a new wording, please re-open this with your draft. Thanks.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

I think that description is clear enough. :) Thanks...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
/(?<![.\-_][Dd]\d+)(\.diff|\.patch)$/

is an invalid lookbehind and led to PCRE compilation failures and plenty of ignored/untested patches:

Warning: preg_match() [function.preg-match]: Compilation failed: lookbehind assertion is not fixed length at offset 17 in pift_test_check_criteria_file() (line 146 of /var/www/drupal.org/htdocs/sites/all/modules/project_issue_file_test/pift.test.inc).

Fixed version:

/(?<![.\-_][Dd]\d)(\.diff|\.patch)$/

EDIT: i.e., I fixed the setting already.

dww’s picture

dww
we/he/they
commented

Thanks sun! Sorry for the drive-by "fix" without looking at the logs and/or confirming it was working. I've got too much on my mind and plate these days to be as careful as I usually try to be. ;)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Please make sure this gets announced to the development list because this broken anyone's patch process that used the previously valid -[dD]7 syntax on their patches which they are now finding being ignored.

dww’s picture

dww
we/he/they
commented

http://lists.drupal.org/pipermail/development/2010-November/036782.html

Status: Fixed » Closed (fixed)

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