Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin created an issue. See original summary.

catch’s picture

Status: Active » Needs review
FileSize
2.32 KB

Posting the patch via killes.

dsnopek’s picture

Here's an alternative pair of patches. I think they are closer to the original D7 commit:

http://cgit.drupalcode.org/prepopulate/commit/?id=16cdb63cc3b256dd785e02...

And there's two of them: one against 6.x-2.x-dev, and the other against the 6.x-2.2 release.

dsnopek’s picture

Do we need a 3rd party to review before some selection of these patches is committed to the Git repo? I'm running the patches from #3 on several client sites and they're working at least in my testing. I'm happy to just commit away if that's cool with everyone. :-)

catch’s picture

@dsnopek I think it's better to get the patches committed sooner than later. I'd have directly committed this to the repo if I'd had time this morning so far and you'd not also independently worked on a patch. We can always post follow-up issues if there's further changes to make, but makes it easier to work on the outstanding issues if the fixed ones aren't sitting in the queue. Also the fact three people have worked on this now, plus the original work on the 7.x patch which is nearly identical means it's been pretty well reviewed.

The USAGE.txt removal looks like a mistake to me in the 7.x commit.

I've removed the hunk referring to that from killes' patch. That gives us an option between just fixing the security issue, vs your patches which backport the full commit.

I'm fine with either, so moving the issue to RTBC.

catch’s picture

Status: Needs review » Reviewed & tested by the community
dsnopek’s picture

Status: Reviewed & tested by the community » Needs work

The USAGE.txt removal looks like a mistake to me in the 7.x commit.

Fair enough!

However, that patch still leaves in a comment that no longer applies:

+++ b/prepopulate.module
@@ -25,9 +25,6 @@ function prepopulate_help($path, $arg) {
   // Provide for accepting base64 encoded fields.

Also, @killes patch applies against 6.x-2.x-dev rather than 6.x-2.2, which is probably the more likely to be installed. I think we should either re-roll the patch to apply to 6.x-2.2, or include two separate patches (it's whitespace changes that make it so the same patch doesn't work for both).

dsnopek’s picture

Status: Needs work » Reviewed & tested by the community

Actually, since @catch is fine with any of the patches, RTBC is probably the right status.

dsnopek’s picture

Status: Reviewed & tested by the community » Fixed

I just went ahead and committed the patch from #5 but renamed to signal that it's security only and works with 6.x-2.x-dev (there's a sort of internal logic that it doesn't remove the invalid comment if it's security only :-)), and both patches from #3.

If anyone wants to make changes or do this different, please feel free to re-open this issue and commit any changes, or open a new issue.

Thanks!

Status: Fixed » Closed (fixed)

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