Problem/Motivation

When there is a large phpstan diff precommit hooks can oom.

#3529504: Fix phpstan errors in UpdatePathTestTrait

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/scripts/dev/c...

Steps to reproduce

Proposed resolution

  • Add an option to set a -1 memory limit.

Remaining tasks

Should we add the memory bypass to anything else?

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3529507

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:

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

Title: St memory limit to -1 for phpstam precommit hooks » Set memory limit to -1 for phpstan precommit hooks
xjm’s picture

Defaulting to -1 is probably bad for CI, but an option would be nice.

nicxvan’s picture

Issue summary: View changes

nicxvan’s picture

Issue summary: View changes
Status: Active » Needs review
xjm’s picture

Title: Set memory limit to -1 for phpstan precommit hooks » Allow setting memory limit to -1 for phpstan precommit hooks

Retitling to make it clear we are not, in fact, trying to take down GitLab.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems like a good improvement for developers.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think we should remove the output as it is not telling you much and I'm not sure that it is helpful.

alexpott’s picture

If this helps a committer commit then +1 - I think the maintenance burden is light and if PHPStan stops supporting the option it'll tell us with an easy error to fix. Also happy to merge https://github.com/alexpott/d8githooks/pull/34/commits/6c03a0ceae34850a9... once this lands.

nicxvan’s picture

Do you mean the print statements?

I had thought it would be helpful to clarify that memory limit is set since i assume most core committers use your precommit hooks and might not see the issue.

I'm happy to remove it too, just clarifying.

alexpott’s picture

Yep the print statements - I'm not sure they add anything.

xjm’s picture

Thanks @alexpott! (@alexpott is the subsystem maintainer for this code.)

nicxvan’s picture

Status: Needs work » Needs review

Done! Thanks

alexpott’s picture

looks good to me... will merge once this rtbc.

xjm’s picture

I had thought it would be helpful to clarify that memory limit is set since i assume most core committers use your precommit hooks and might not see the issue.

We are a small group and whoever commits the followup to the git hooks will probably just tell the team. :)

I'll try to manually test this together with the PR later.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Feedback from @alexpott appears to be addressed.

xjm’s picture

Assigned: Unassigned » xjm
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Sorry, it was NR for manual testing with the upstream PR. I should have been explicit. I can do this, but am also fine for someone else to if they beat me to it.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Tested manually.

Without fix

  1. Locally setting memory_limit = 256M
  2. Reverting #3533300: PHPStan baseline is out of sync.
  3. Applying patch from that issue.
  4. Attempt to commit it (including running pre-commit checks).

Result:

Running PHPStan on *all* files.
PHP Fatal error:  Allowed memory size of 268435456 bytes exhausted (tried to allocate 181264717 bytes) in phar:///Users/xjm/git/maintainer/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/ResultCache/ResultCacheManager.php on line 172
Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 181264717 bytes) in phar:///Users/xjm/git/maintainer/vendor/phpstan/phpstan/phpstan.phar/src/Analyser/ResultCache/ResultCacheManager.php on line 172

PHPStan process crashed because it reached configured PHP memory limit: 256M
Increase your memory limit in php.ini or run PHPStan with --memory-limit CLI option.

PHPStan: failed

With fix

  1. Locally setting memory_limit = 256M
  2. Reverting #3533300: PHPStan baseline is out of sync.
  3. Committing the MR from this issue.
  4. Fetching the latest changes in the d8githooks
  5. Manually patching precommit in the d8githooks with the flag from that MR and committing the result.
  6. Recommitting the patch from the baseline update (with precommit checks).

Result: Baseline update committed with full PHPStan scan.

Now it is RTBC. :)

alexpott’s picture

Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 2ffd3412fe7 to 11.x and 4842c65b535 to 11.2.x and ca127551d4f to 10.6.x and 642eb99e04e to 10.5.x. Thanks!

Backported to 10.5.x as this is non-runtime code and it does not hurt to have everything in sync.

  • alexpott committed 642eb99e on 10.5.x
    Issue #3529507 by nicxvan, xjm, alexpott: Allow setting memory limit to...

  • alexpott committed ca127551 on 10.6.x
    Issue #3529507 by nicxvan, xjm, alexpott: Allow setting memory limit to...

  • alexpott committed 4842c65b on 11.2.x
    Issue #3529507 by nicxvan, xjm, alexpott: Allow setting memory limit to...

  • alexpott committed 2ffd3412 on 11.x
    Issue #3529507 by nicxvan, xjm, alexpott: Allow setting memory limit to...
xjm’s picture

Assigned: xjm » Unassigned

Status: Fixed » Closed (fixed)

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