Problem/Motivation

#3609853 added PerformanceAnalyzer::stripComments(), used by scanFileForPatterns() and scanCodeFileForPerfPatterns(). It blanks /* ... */, // ... and lines starting with '*' with regular expressions. Three cases are still wrong on current 1.x (reproduced with drush audit:run performance on probe files):

  1. Twig comments. scanFileForPatterns() also scans .html.twig files for the TWIG_DEBUG_* patterns, but stripComments() does not know {# ... #}: "{# {{ kint(content) }} #}" is reported as TWIG_DEBUG_KINT, and a multi-line {# ... #} containing {{ dump(node) }} as TWIG_DEBUG_DUMP. #3606540 fixed exactly this in audit_twig's TwigAnalyzer, but audit_performance has its own detection that was not updated.
  2. PHP '#' line comments: "# var_dump($x);" is reported as DEBUG_VAR_DUMP and "$y = 1; # print_r($y);" as DEBUG_PRINT_R.
  3. '//' inside strings (false negative, a regression of #3609853): the '//' regex has no string awareness, so on "$u = 'https://example.com'; dpm($u);" everything after "https:" is blanked and the dpm() is never reported. The same applies to '/*' inside strings, such as a glob '/*.php'.

In addition, the context checks (hasCacheContextInMethod(), hasCacheMetadataInClass(), and the loop, pagination, access check and query checks behind isContextualPerfIssue()) still read the raw content, so a commented-out foreach or '#cache' still changes their result.

A naive '#' regex is not an option: '#cache', '#markup' and the like are everywhere in Drupal code inside strings.

Proposed resolution

  • Add Drupal\audit\StaticAnalysis\CommentStripper in the base module, so the other static scanners can reuse it. For PHP it uses token_get_all() and blanks T_COMMENT and T_DOC_COMMENT tokens, which covers //, #, /* */ and docblocks while leaving strings and PHP 8 attributes (#[...]) untouched. For Twig it blanks {# ... #}, including multi-line comments. Comments are replaced with spaces, so offsets and line numbers do not change.
  • PerformanceAnalyzer::stripComments() picks the PHP or Twig stripper from the file extension. Twig templates no longer get // and /* */ stripped, since those are not comments in Twig (and debug calls inside a script or style block do run).
  • TWIG_DEBUG_CONTEXT intentionally matches "{# debug" comments, so it is flagged 'matches_comments' and keeps running on the raw content.
  • The context checks receive the stripped content; the code context shown in the report still comes from the raw lines.
  • Tests: CommentStripperTest, and a PerformanceAnalyzer test that scans a PHP file and a Twig template with the cases above and asserts only the real findings are reported.

This also resolves the '//' inside strings regression described above, which was going to be reported as a separate issue.

Longer term, the Twig debug detection could be removed from audit_performance and left to audit_twig; that is not part of this fix.

Remaining tasks

  • Review the merge request.

Comments

trebormc created an issue. See original summary.

  • trebormc committed dc2ccef2 on 1.x
    Issue #3626581 followup by trebormc: Split TWIG_DEBUG_CONTEXT so a...

  • trebormc committed 974fdc57 on 1.x
    Issue #3626581 by trebormc: Ignore Twig {# #} and PHP '#' comments...
trebormc’s picture

Status: Active » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.