Test Carbon mixin #1857

spawnia · 2024-02-15T10:59:55Z

Added or updated tests
Documented user facing changes

Changes

I am trying to reproduce an issue in a project of mine that uses Carbon mixins.

~~After updating from larastan v2.8.1 to v2.9.0~~ errors such as the following started appearing:

~~Update: the issue also exists with v2.8.1, sorry. I am digging further to find what caused the issue.~~

Second update: I tried updating the dependencies one by one. It was indeed the update from Larastan that was responsible.

bfranke@MLLNB61:~/projects/nemo-api (update-deps)*$ make stan
$( [ -f /.dockerenv ] && echo "" || echo "docker-compose exec nemo-api-php") vendor/bin/phpstan analyse --memory-limit=2048M
Note: Using configuration file /var/www/phpstan.neon.
 836/836 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 [OK] No errors                                                                                                         

bfranke@MLLNB61:~/projects/nemo-api (update-deps)*$ make shell
www-data@482021448d96:/var/www$ composer update larastan/larastan
Loading composer repositories with package information
Updating dependencies
Lock file operations: 0 installs, 1 update, 0 removals
  - Upgrading larastan/larastan (v2.8.1 => v2.9.0)
Writing lock file
Installing dependencies from lock file (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
  - Upgrading larastan/larastan (v2.8.1 => v2.9.0): Extracting archive
Generating optimized autoload files
> Illuminate\Foundation\ComposerScripts::postAutoloadDump
> @php artisan package:discover --ansi

   INFO  Discovering packages.

  barryvdh/laravel-ide-helper ................................................................................................................. DONE
  bensampo/laravel-enum ....................................................................................................................... DONE
  erjanmx/laravel-migrate-check ............................................................................................................... DONE
  intervention/image .......................................................................................................................... DONE
  itsgoingd/clockwork ......................................................................................................................... DONE
  laravel/tinker .............................................................................................................................. DONE
  mll-lab/laravel-comment ..................................................................................................................... DONE
  mll-lab/laravel-graphiql .................................................................................................................... DONE
  mll-lab/laravel-graphql-voyager ............................................................................................................. DONE
  mll-lab/laravel-utils ....................................................................................................................... DONE
  nesbot/carbon ............................................................................................................................... DONE
  nunomaduro/collision ........................................................................................................................ DONE
  nunomaduro/laravel-mojito ................................................................................................................... DONE
  nunomaduro/termwind ......................................................................................................................... DONE
  nuwave/lighthouse ........................................................................................................................... DONE
  sentry/sentry-laravel ....................................................................................................................... DONE
  spatie/laravel-activitylog .................................................................................................................. DONE
  spatie/laravel-medialibrary ................................................................................................................. DONE
  staudenmeir/eloquent-has-many-deep .......................................................................................................... DONE
  thedoctor0/laravel-factory-generator ........................................................................................................ DONE

131 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
phpstan/extension-installer: Extensions installed
> bensampo/laravel-enum: installed
> composer/composer: installed
> ekino/phpstan-banned-code: installed
> larastan/larastan: installed
> mll-lab/php-utils: installed
> nesbot/carbon: installed
> phpstan/phpstan-deprecation-rules: installed
> phpstan/phpstan-mockery: installed
> phpstan/phpstan-phpunit: installed
> thecodingmachine/phpstan-safe-rule: installed
> Illuminate\Foundation\ComposerScripts::postUpdate
> @php artisan ide-helper:generate
A new helper file was written to _ide_helper.php
> @php artisan ide-helper:meta
A new meta file was written to .phpstorm.meta.php
> @php artisan lighthouse:ide-helper
Wrote schema directive definitions to /var/www/schema-directives.graphql.
Wrote definitions for programmatically registered types to /var/www/programmatic-types.graphql.
Wrote PHP definitions to /var/www/_lighthouse_ide_helper.php.

It is recommended to add them to your .gitignore file.
> @php artisan vendor:publish --tag=strict-stubs --force

bfranke@MLLNB61:~/projects/nemo-api (update-deps)*$ make stan
$( [ -f /.dockerenv ] && echo "" || echo "docker-compose exec nemo-api-php") vendor/bin/phpstan analyse --memory-limit=2048M
Note: Using configuration file /var/www/phpstan.neon.
 836/836 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ------------------------------------------------------------------------------- 
  Line   app/Models/File.php
 ------ -------------------------------------------------------------------------------
  55     Static call to instance method Carbon\Carbon::valid().                         
         ✏️  app/Models/File.php:55
  63     Static call to instance method Carbon\Carbon::createFromHyphenlessDateTime().
         ✏️  app/Models/File.php:63
  73     Static call to instance method Carbon\Carbon::createFromHyphenlessDateTime().
         ✏️  app/Models/File.php:73
 ------ -------------------------------------------------------------------------------

 ------ --------------------------------------------------------
  Line   app/Models/HistoryModel.php
 ------ --------------------------------------------------------
  112    Static call to instance method Carbon\Carbon::valid().
         ✏️  app/Models/HistoryModel.php:112
  278    Static call to instance method Carbon\Carbon::valid().
         ✏️  app/Models/HistoryModel.php:278
 ------ --------------------------------------------------------

 ------ -------------------------------------------------------------------------------
  Line   app/Models/MllGene.php
 ------ -------------------------------------------------------------------------------
  156    Static call to instance method Carbon\Carbon::createFromHyphenlessDateTime().
         ✏️  app/Models/MllGene.php:156
  166    Static call to instance method Carbon\Carbon::createFromHyphenlessDateTime().
         ✏️  app/Models/MllGene.php:166
 ------ -------------------------------------------------------------------------------

 ------ --------------------------------------------------------
  Line   database/factories/MllGeneFactory.php
 ------ --------------------------------------------------------
  68     Static call to instance method Carbon\Carbon::valid().
         ✏️  database/factories/MllGeneFactory.php:68
 ------ --------------------------------------------------------


 [ERROR] Found 8 errors

  Line   database/factories/MllGeneFactory.php
 ------ --------------------------------------------------------
  68     Static call to instance method Carbon\Carbon::valid().

The code under question:

use Carbon\Carbon;
...
            'valid_to' => Carbon::valid(),

The relevant part of the mixin:

use Carbon\Carbon;

/** @mixin Carbon */
final class CarbonMixin
{
    public static function valid(): \Closure
    {
        return static function (): Carbon {
            // @phpstan-ignore-next-line This never returns false, we use static values
            return Carbon::create(9999, 12, 31, 23, 55, 55);
        };
    }
}

It is registered in a service provider like this:

    public function boot(): void
    {
        Carbon::mixin(new CarbonMixin());
    }

I can confirm that the mixin is recognized, as changing the method name to something nonsensical triggers the following error:

  Line   database/factories/MllGeneFactory.php
 ------ -----------------------------------------------------------
  68     Call to an undefined static method Carbon\Carbon::asdf().

Unfortunately, I am unable to reproduce the issue in a test case here. Can you help?

Breaking changes

Not yet.

canvural · 2024-02-15T14:00:03Z

Are you also using PHPStan strict rules?

spawnia · 2024-02-15T14:19:25Z

Are you also using PHPStan strict rules?

No. I am using https://github.com/phpstan/extension-installer, so I double checked that the strict rules package is not present in vendor.

calebdw · 2024-02-15T14:39:40Z

If you're using the extension-installer then the Carbon extension should be automatically installed. Try testing the Carbon mixin without Larastan, and if the issue still persists then it's something on their end.

calebdw · 2024-02-15T14:55:09Z

tests/phpstan-tests.neon

@@ -1,6 +1,8 @@
 includes:
 - ../extension.neon
+ - ../vendor/nesbot/carbon/extension.neon


What happens if you remove this line?

The tests I added in Larastan pass with or without the setting.

Regardless, we shouldn't be testing their extension in our tests---personally I'm of the mindset that Larastan should remove support for the Carbon mixins as Carbon provides their own extension for their own mixins

I added a comment as to why I think it is useful for testing: d606311

I still don't think this is necessary/required---our job is to test our own extension, not what other users might install in their application.

I would argue that Larastan does not exist in isolation, thus a more integration-like test setup is useful.

I could see that argument when running the e2e tests, but not when testing the code specific to this package

szepeviktor · 2024-02-15T15:27:37Z

Could you fix another typo? ./tests/Type/data/model-properties.php:80 "overriden"

spawnia · 2024-02-15T16:56:32Z

If you're using the extension-installer then the Carbon extension should be automatically installed. Try testing the Carbon mixin without Larastan, and if the issue still persists then it's something on their end.

Thanks for the feedback. I tried adding the following to my composer.json:

    "extra": {
        "phpstan/extension-installer": {
            "ignore": [
                "nesbot/carbon"
            ]
        }
    }

The resulting errors are exactly the same as before. As I have outlined in the second update to the description, the update from larastan v2.8.1 to v2.9.0 is indeed the single change that causes the issue to appear. Not saying larastan is wrong or buggy, but that triggers the error.

spawnia · 2024-02-15T16:57:17Z

tests/Type/GeneralTypeTest.php

Temporarily removed to allow CI to pass and run on all versions.

spawnia · 2024-02-15T16:58:44Z

Are you also using PHPStan strict rules?

Here are the included extensions

phpstan/extension-installer: Extensions installed
> bensampo/laravel-enum: installed
> composer/composer: installed
> ekino/phpstan-banned-code: installed
> larastan/larastan: installed
> mll-lab/php-utils: installed
> nesbot/carbon: installed
> phpstan/phpstan-deprecation-rules: installed
> phpstan/phpstan-mockery: installed
> phpstan/phpstan-phpunit: installed
> thecodingmachine/phpstan-safe-rule: installed

calebdw · 2024-02-15T17:01:36Z

The resulting errors are exactly the same as before. As I have outlined in the second update to the description, the update from larastan v2.8.1 to v2.9.0 is indeed the single change that causes the issue to appear.

Which version of Carbon are you on?

Also, you should try analyzing the Carbon mixin without Larastan using only the Carbon extension

# Conflicts: # tests/Type/GeneralTypeTest.php

Test Carbon mixin

66de05a

Ensure level max is used

7b74bec

calebdw reviewed Feb 15, 2024

View reviewed changes

spawnia added 4 commits February 15, 2024 17:49

Comment reason for adding Carbon extension

d606311

overridden

9f89516

Temporarily disable failing test

a7fa3ec

Temporarily delete failing test class

6c58ecf

spawnia commented Feb 15, 2024

View reviewed changes

tests/Type/GeneralTypeTest.php Outdated

Copy link

Contributor Author

spawnia Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporarily removed to allow CI to pass and run on all versions.

Merge branch '2.x' into test-carbon-mixin

1931c32

# Conflicts: # tests/Type/GeneralTypeTest.php

canvural force-pushed the 2.x branch from 2e7b3fe to 56d7123 Compare April 16, 2024 11:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test Carbon mixin #1857

Test Carbon mixin #1857

spawnia commented Feb 15, 2024 •

edited

canvural commented Feb 15, 2024

spawnia commented Feb 15, 2024

calebdw commented Feb 15, 2024

calebdw Feb 15, 2024

spawnia Feb 15, 2024

calebdw Feb 15, 2024

spawnia Feb 15, 2024

calebdw Feb 15, 2024

spawnia Feb 19, 2024

calebdw Feb 19, 2024

szepeviktor commented Feb 15, 2024

spawnia commented Feb 15, 2024

spawnia Feb 15, 2024

spawnia commented Feb 15, 2024 •

edited

calebdw commented Feb 15, 2024 •

edited

Test Carbon mixin #1857

Are you sure you want to change the base?

Test Carbon mixin #1857

Conversation

spawnia commented Feb 15, 2024 • edited

canvural commented Feb 15, 2024

spawnia commented Feb 15, 2024

calebdw commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szepeviktor commented Feb 15, 2024

spawnia commented Feb 15, 2024

Choose a reason for hiding this comment

spawnia commented Feb 15, 2024 • edited

calebdw commented Feb 15, 2024 • edited

spawnia commented Feb 15, 2024 •

edited

spawnia commented Feb 15, 2024 •

edited

calebdw commented Feb 15, 2024 •

edited