Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove null from parse and rawParse return types #2931

Merged
merged 2 commits into from Jan 31, 2024

Conversation

jnoordsij
Copy link
Contributor

@jnoordsij jnoordsij commented Jan 31, 2024

When doing some static analysis on a partial test of migrating my Laravel application to Laravel 11 + Carbon 3, I noticed that PHPStan (level max) reported nullability warnings on all calls related to Carbon::parse, that were previously not there. This is due to the fact that the v2 docblock uses static (without null) as return type:

/**
* Create a carbon instance from a string.
*
* This is an alias for the constructor that allows better fluent syntax
* as it allows you to do Carbon::parse('Monday next week')->fn() rather
* than (new Carbon('Monday next week'))->fn().
*
* @param string|DateTimeInterface|null $time
* @param DateTimeZone|string|null $tz
*
* @throws InvalidFormatException
*
* @return static
*/
public static function parse($time = null, $tz = null)

Diving a little further into the method (and it's near-equivalent 3.x counterpart), I'm unsure why null has been added as possible return type. Given that all tests seem to be running still fine after enforcing non-nullability and the old docblock already stated the expected output to be non-null, I think sticking with a non-null output type should be the way to go for the 3.x version. Moreover, looking at the method bodies, I see little possibility for null to be an actual output.

This has the benefits of being much easier to handle both practically and statically within application, without requiring careful thinking about possible null values.

Furthermore, I've altered the rawParse signature to use static rather than self, given that looks more accurate, specific and in line with the parse method.

@jnoordsij
Copy link
Contributor Author

Regarding the PHPStan failure: this seems to be caused by PHPStan now actually knowing the outcome of the parse method in the test is a Carbon instance, rather than a possible null, and thus complaining. I'm willing to look for a fix, but not entirely sure how to proceed. Possibly some kind of hint in the docblock of CarbonInterface should be added, that notifies the month enum as suitable type for setting this property is required.

@kylekatarnls
Copy link
Collaborator

The nullable comes from the fact that we allow to customize $parseFunction which was mainly to help transitioning from packages extending the Carbon class. But as extending Carbon (or any class coming from vendor dependencies) is not recommended anyway, we can consider making it non-nullable. It's a bit short for 3.0.0. I will evaluate.

Regarding the PHPStan failure

This one is about ->month that now can be set to either int or Month enum which should be in theory represented by:

@property-read int $month
@property-write Month|int $month

Sadly while this is actually the reality, phpDocumentor does not accept to have both @property-read and @property-write for the same property.

So this error can just be added to the ignore list in phpstan.neon.

@jnoordsij
Copy link
Contributor Author

Thanks for looking into this!

Regarding the behavior: I understand adding the nullable a bit more now, seems like a tricky situation! I think it's weighing the possible benefits of slightly easier migration for paths/functionality that sounds a bit like an (unsupported) edge-case to me vs. something that will probably only bother those with quite strict type checking, which is most likely also a relative small group of users. I'll leave it up to you to weigh those factors.

I've added an ignore clause for the static error as suggested.

@kylekatarnls kylekatarnls merged commit 7ae1c6f into briannesbitt:3.x Jan 31, 2024
20 checks passed
@jnoordsij jnoordsij deleted the non-null-parse branch February 1, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants