Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Elijah Lynn’s picture

Elijah Lynn’s picture

Status: Active » Needs review
aaronott’s picture

Status: Needs review » Reviewed & tested by the community

Patch applies cleanly.

Looks good to me.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

Thanks! This all looks good except:

- *   The type of the item (i.e. theme, theme_engine, module, profile).
+ *   The type of the item (e.g. theme, theme_engine, module, profile).

People are always mistaking i.e. for e.g. (or vice versa) and they are nearly always (as in this case) also punctuated incorrectly (there needs to be a comma after either i.e. or e.g.). So please change it to read "(for example, theme, theme_engine, module, or profile). Actually though... come to think of it, i.e. is actually correct here, since this function is not supposed to work for anything except for those 4 choices. So let's actually just take the i.e. out entirely, because then it will be even clearer that these are the only 4 choices.

Also, by policy we need to make these fixes in Drupal 8.x first and then backport to 7.x. At least one of them is in 8.x boostrap.inc; the others may be in other files.

Elijah Lynn’s picture

Status: Needs work » Needs review
FileSize
972 bytes

Thanks Jennifer,

The first two in DrupalCacheArray::persist do not exist in the D8 bootstrap.inc but the last two in drupal_get_filename() do exist. Here is a patch for those last two in D8.

Elijah Lynn’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

Here is the updated patch for D7 (minus e.g.).

Status: Needs review » Needs work

The last submitted patch, drupal-grammar-bootstramp.inc-2038059-6.patch, failed testing.

Elijah Lynn’s picture

For future reference, is there anything I can add to a patch to have it test against a certain version of Drupal?

jhodgdon’s picture

Status: Needs review » Needs work

RE #8: no. Uploading patches to the wrong version is discouraged -- see https://drupal.org/node/1319154#multiple-versions

If you feel you must upload a patch for a different version ahead of time, there is a suffix to make it not test (it is described in the help text under the Attachment field on issue comments).

Regarding the patch in #5, CacheArray and CacheCollector have the same problems in Drupal 8 (they are just in different files, not in bootstrap.inc) that you identified in Drupal 7. So please patch those files. Thanks!

Elijah Lynn’s picture

Thanks for the tip on no testing trick.

Attached is a patch for D8 that also corrects CacheArray and CacheCollector.

Elijah Lynn’s picture

Status: Needs work » Needs review
Elijah Lynn’s picture

That was a bad patch, use this instead.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I'll get this patch committed shortly.

Please wait to make the D7 patch until that's done, because if you upload a D7 "do not test" patch now, someone will need to download your patch, change the name, and re-upload in order for it to be tested (and it needs to be tested before it can be committed, even though it's just API docs).

Elijah Lynn’s picture

Thanks Jennifer!

And thanks for the explanation on waiting for the D7 patch, make perfect sense!

Cheers,

Elijah

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks again! Committed to 8.x. Ready for backport to 7.x now.

Elijah Lynn’s picture

Thanks Jennifer,

Here is the same patch from #6.

Cheers!

Elijah Lynn’s picture

Status: Patch (to be ported) » Needs review
longwave’s picture

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

Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x.

Status: Fixed » Closed (fixed)

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