Site admins sometimes get memory exhausted errors and then it's hard to find which specific check is causing the problem (e.g. #2815487: Fatal error when running the review).

If security review logged the beginning of each step then it would be easier to find the problem.

CommentFileSizeAuthor
#14 2897949-14.patch686 bytessmustgrave
#6 2897949-log-beginning-of-steps-5.patch1.53 KBgreggles
#2 2897949-log-beginning-of-steps.patch1.39 KBgreggles

Issue fork security_review-2897949

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles created an issue. See original summary.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
Status: Active » Needs review
FileSize
2897949-log-beginning-of-steps.patch1.39 KB
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented

There is a somewhat unrelated change here of ! to @ for placeholders and using the variable (title, name) value to guide the name of the placeholder.

This will break any translations of those strings, but that seems OK/worth it to me.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented

Typo in here of using @name twice when it should be @title in one case.

dpintats’s picture

dpintats CreditAttribution: dpintats commented

Looks like the line $variables = array('@name' => $check['title'], '@name' => $check_name); has '@name' as both placeholders. The rest looks good though.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
FileSize
2897949-log-beginning-of-steps-5.patch1.53 KB

Updated to reflect the point David Pintado found :)

  • greggles committed 27d431b on 7.x-1.x 
    Issue #2897949 by greggles, dpintats: Log the beginning of a check (in...
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at CARD.com commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

Now committed to 7.x.

Would be ideal to port this to 8.x, so updating metadata.

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
CreditAttribution: vuil at Bulpros Consulting commented
Version: 8.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Fixed

@greggles
Sorry but this patch (7.x) functionality does not need to be integrated on 8.x-1.x-dev branch.

I close the issue as Fixed, and set it back to 7.x-1.x-dev version. Thank you!

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Fixed » Patch (to be ported)

HI Ilcho - thanks for your perspective. Could you clarify why you think that?

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Version: 8.x-1.x-dev » 2.0.x-dev
Status: Patch (to be ported) » Postponed (maintainer needs more info)

Wonder if or where this should go into 2.x branch now that things have changed?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at Morris Animal Foundation commented
Status: Postponed (maintainer needs more info) » Patch (to be ported)

I think it's still useful as people still report running out of memory during execution of the checks.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at Morris Animal Foundation commented

And sure, 2.0.x-dev seems like a good spot for it to me. It's not a critical feature to support in all branches.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Patch (to be ported) » Needs review
FileSize
2897949-14.patch686 bytes

This covers it?

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at Morris Animal Foundation commented

I didn't test it, but yes, that looks good to me.

smustgrave opened merge request !9

  • smustgrave committed b4b2da3 on 2.0.x 
    Issue #2897949: Log the beginning of a check (in addition to end) to...
smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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