Problem/Motivation

The composer phpcs exceeds the php memory limit if it's 256MB or lower.

Steps to reproduce

Set memory limit to 256M and run composer phpcs:

The PHP_CodeSniffer "phpcs" command ran out of memory.
Either raise the "memory_limit" of PHP in the php.ini file or raise the memory limit at runtime
using `phpcs -d memory_limit=512M` (replace 512M with the desired memory limit).

Proposed resolution

Set memory limit to -1 (unlimited) for phpcs and phpcbf commands.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3549208

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

Comments

mstrelan created an issue. See original summary.

mstrelan opened merge request !13333

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
cilefen’s picture

cilefen commented

Why not -1?

mstrelan’s picture

mstrelan commented

Currently it passes with 384 MB so the current requirement is somewhere between 256 and 384. If we set it to -1 and the process starts requiring more than 512 MB (or possibly much more) then that's a good indicator we might have a problem. Maybe phpcs started scanning node_modules or vendor or something like that.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems straight forward and explanation makes sense

xjm’s picture

xjm
she/her
Primary language English
commented
Related issues: +#3538145: Optionally bypass the memory limit for PHPCS in the core commit check script

So we were taking the approach of allowing the memory limit to be bypassed when running the commit check script. I'm not sure about harcoding a specific memory limit like this. Can the developer still override it to something higher or lower?

I'm not sure scanning all of core is the default operation (I'm assuming the IS means it OOMs when scanning all of core).

mstrelan’s picture

mstrelan commented

I'm not sure scanning all of core is the default operation (I'm assuming the IS means it OOMs when scanning all of core).

I had never considered that you could pass additional arguments. The default is to scan all of core, but you can indeed pass a path to scan instead.

I'm not sure about harcoding a specific memory limit like this. Can the developer still override it to something higher or lower?

AFAIK the supported way to set the memory limit for this command is COMPOSER_MEMORY_LIMIT=512M composer phpcs. I tested this on this branch with 16M and it OOM'd straight away, so I guess that's working.

$ COMPOSER_MEMORY_LIMIT=16M composer phpcs
PHP Fatal error:  Allowed memory size of 16777216 bytes exhausted (tried to allocate 204800 bytes) in /usr/share/php/Symfony/Component/Process/Process.php on line 921

I also tried the inverse, by setting the memory limit in composer.json to 16M and 512M in the env var, but that also OOM'd.

TL;DR the developer can set it lower but not higher. So maybe -1 is the way to go? But in saying that, the command is not particularly useful for scanning anything other than core, since core has its own specific rules and path inclusions.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Ooh yeah, if we can't override it to be higher, I think that's not good. Thanks!

The commit check script fix has landed BTW, so committers will already be -1ing up our machines by default when we commit stuff.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Needs work » Needs review

Rebased and updated to -1 instead of 512M

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I'm not sure there's a lot to do for reviewing this one, but for what it's worth I did the following:

  • Verified that the commands ran correctly on my local. There were no issues.
  • Validated the new argument with the PHPCS documentation. It looks good.
  • Checked to see if we already advise running with no memory limit in our documentation about PHPCS, similar to how we do for PHPStan. We don't, but I don't think that's a problem.

It looks good to me.

  • catch committed 2258dcf5 on 11.4.x 
    task: #3549208 Set memory limit for  command

By: mstrelan
By: cilefen...

  • catch committed 2306e467 on 11.x 
    task: #3549208 Set memory limit for  command

By: mstrelan
By: cilefen...

  • catch committed e0d0ed79 on main 
    task: #3549208 Set memory limit for  command

By: mstrelan
By: cilefen...

catch closed merge request !13333

catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main and cherry-picked back through to 11.4.x, thanks!

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.