phpro / grumphp

A PHP code-quality tool
MIT License
4.15k stars 433 forks source link

Serialization of 'WeakMap' is not allowed after update monolog to 2.9.0 #1065

Closed Eduardo-Morales-Alberti closed 1 year ago

Eduardo-Morales-Alberti commented 1 year ago
Q A
Version 1.5.1
Bug? yes
New feature? no
Question? no
Documentation? no
Related tickets comma-separated list of related tickets

My configuration

grumphp:
  git_hook_variables:
    EXEC_GRUMPHP_COMMAND: docker-compose run -e'PHP_ERROR_REPORTING=E_ALL & ~E_DEPRECATED' --rm -T php
  ascii:
    failed: grumphp-ascii/grumpy.txt
    succeeded: grumphp-ascii/happy.txt
  extensions:
    - GrumphpDrupalCheck\ExtensionLoader
    - Metadrop\PhpCompatibilityTask\ExtensionLoader
  hooks_dir: ~
  tasks:
    git_commit_message:
      matchers:
        Must contain gitlab reference (if no reference use 0): '/(^SDXP2-[0-9]+:|^Merge branch.)/'
        Space and uppercase after reference: '/(^SDXP2-[0-9]+: [A-Z]|^Merge branch.)/'
      enforce_no_subject_trailing_period: true
      max_subject_width: 140
      case_insensitive: false
    phplint: ~
    yamllint: ~
    composer: ~
    jsonlint: ~
    drupalcheck: ~
    phpcs:
      encoding: utf-8
      triggered_by:
        - php
        - inc
        - module
        - install
        - profile
        - theme
        - feature
        - info
        - txt
        - md
        - js
        - test
      standard: Drupal,DrupalPractice
      ignore_patterns:
        - ./solr/*
    php_compatibility:
      testVersion: "8.1"
      triggered_by:
        - php
        - inc
        - module
        - install

Steps to reproduce: It happens after update monolog to the last version 2.9.0.

# Generate empty folder
mkdir tmp
cd tmp
git init
echo "vendor" > .gitignore
pbpaste > grumphp.yml
composer require --dev phpro/grumphp

# Run GrumPHP:
git add -A && git commit -m"Test"

Result:

GrumPHP detected a pre-commit command.
GrumPHP is sniffing your code!

Running tasks with priority 0!
==============================

Running task 1/7: phplint... 
Running task 2/7: yamllint... 
Running task 3/7: composer... 
Running task 4/7: jsonlint... 
Running task 5/7: drupalcheck... 
Running task 6/7: phpcs... 
Running task 7/7: php_compatibility... 

Running task 1/7: phplint... 
Running task 2/7: yamllint... ✔
Running task 3/7: composer... 
Running task 4/7: jsonlint... 
Running task 5/7: drupalcheck... 
Running task 6/7: phpcs... 
Running task 7/7: php_compatibility... 

GrumPHP detected a commit-msg command.
GrumPHP is sniffing your code!

Running tasks with priority 0!
==============================

Running task 1/1: git_commit_message... 

Running task 1/1: git_commit_message... ✘

git_commit_message
==================

                   -:
                  -oo:
                `/oooo/`
               -+ooooooo:
             -+oooooooooo+-
           -+oooooooooooooo+-
         -+ooooooo/.`/ooooooo+-
       ./ooooooo+.    ./ooooooo+-
     `/ooooooooo        ./ooooooo/.
    -ooooooooooo:`        ./ooooooo:
   /ooooooo/ooooo+-`        -+oooooo/`
  /oooooo/` `:ooooo+-`       `/oooooo+`
 :oooooo:     `/ooooo+-`       :oooooo/
.oooooo:       `+oooooo+-`      :oooooo-
/oooooo      `:oooooooooo+.      +ooooo+
+ooooo/     -+ooooo:./ooooo/.    :oooooo
+ooooo/   .+ooooo/.   .+ooooo/`  :oooooo
/oooooo-`:ooooo/.       -oooooo-`+ooooo+
.ooooooo+ooooo:          `+ooooo+oooooo-
 /ooooooooooo+            .ooooooooooo+
 `/oooooooooo+            `oooooooooo+`
   /oooooooooo-          `+ooooooooo/`
    -+ooooooooo/.      `-+ooooooooo-
      -+oooooooooo+///+ooooooooo+:`
        .:+oooooooooooooooooo+:.
           `.:/++oooooo++/:-`

Unsupported callable: Serialization of 'WeakMap' is not allowed
To skip commit checks, add -n or --no-verify flag to commit command
veewee commented 1 year ago

Looks related to this change in Monolog: https://github.com/Seldaek/monolog/pull/1753

Weakmap's cannot be serialized. Maybe you can open up an issue in monolog to investigate another approach over using weakmaps?

serialize(new WeakMap())

Exception with message 'Serialization of 'WeakMap' is not allowed'
Eduardo-Morales-Alberti commented 1 year ago

I opened it https://github.com/Seldaek/monolog/issues/1792 but I am not sure how to fill it.

stof commented 1 year ago

Why trying to serialize the logger instance though ? I don't think Monolog ever promised that the logger is serializable (many handlers will not be serializable either)

veewee commented 1 year ago

To run processes in parallel, we use amphp/parallel in combination with serializable-closure. This serializable-closure is serializing the context of the closure, which might include the logger.

For example, the git_commit_message here will have a dependency to gitonomy, which is configured with a logger (monolog). Therefore, the serializable-closure will try to serialize both gitonomy and monolog into it's context. It's not like we are actively willing to serialize the logger in there, but it's part of the dependency tree.

Eduardo-Morales-Alberti commented 1 year ago

Seems to be fixed by @Seldaek on the next release https://github.com/Seldaek/monolog/issues/1792 https://github.com/Seldaek/monolog/milestone/6?closed=1

veewee commented 1 year ago

Cool, seems fixed indeed! Thanks for reporting and the follow up!