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

disallowedLooseComparison - allow only DateTime/DateTimeImmutable #232

Open
realjjaveweb opened this issue Nov 10, 2023 · 9 comments
Open

Comments

@realjjaveweb
Copy link

disallowedLooseComparison is a nice rule and can be applicable to most of the cases, except DateTime/DateTimeImmutable

Comparing these 2 using comparing operators started in PHP 5.2.2, and is documented even in the current documentation:

Note:
DateTimeImmutable and DateTime objects can be compared using comparison operators.

If we dig deeper in the php-src - https://github.com/php/php-src/blob/master/ext/date/php_date.c
we can see that date_object_handlers_date.compare = date_object_compare_date; and date_object_handlers_immutable.compare = date_object_compare_date; have both the same comparator which leads us to timelib_time_compare in https://github.com/php/php-src/blob/master/ext/date/lib/timelib.c#L75 where we can see it DOES compare microseconds which is something some people may worry about.

In any case - I understand why PHP docs for object comparison say this:

When using the comparison operator (==), object variables are compared in a simple manner, namely: Two object instances are equal if they have the same attributes and values (values are compared with ==), and are instances of the same class.

It's because the == allows comparing objects with nested objects. But in many opinion worlds that is still bad and one should rather specify specifically what is to be compared => however that is not the case of DateTime/DateTimeImmutable.

So the core point is - one should be able to make an exception for disallowedLooseComparison - have it option something like "allowDateTime" or maybe just allow it by default.

Final question is whether this exception should only apply to the core classes, or, should also apply to any classes that extend them (like e.g. Carbon).

@ondrejmirtes
Copy link
Member

Hi, you can solve that today by crafting the right regex to ignore specific errors like this: https://phpstan.org/user-guide/ignoring-errors#ignoring-in-configuration-file

@realjjaveweb
Copy link
Author

realjjaveweb commented Nov 27, 2023

To ignore some custom rule that would e.g. force == for \DateTime(Immutable) the regex would go something like this:
in the file "phpstan.neon.dist" (in your app root):

parameters:
    # ...
    ignoreErrors:
        # ...
        - '#Some Sentence You Want To Ignore\.#'

@ondrejmirtes
Copy link
Member

Where is Cannot compare DateTime instances with === coming from? That's not how the message looks like in phpstan-strict-rules.

@realjjaveweb
Copy link
Author

@ondrejmirtes
Oh sorry - that's a custom rule - I will remove that comment to not confuse future visitors... it was irrelevant. 🙈

The core point is that following

Loose comparison via "==" is not allowed. 💡 Use strict comparison via "===" instead.

Should not be triggered for \DateTime & \DateTimeImmutable since PHP has custom comparing implementation for these 2.

@whataboutpereira
Copy link

To ignore some custom rule that would e.g. force == for \DateTime(Immutable) the regex would go something like this: in the file "phpstan.neon.dist" (in your app root):

parameters:
    # ...
    ignoreErrors:
        # ...
        - '#Some Sentence You Want To Ignore\.#'

Not sure that would work well because the error is simply Loose comparison via "==" is not allowed. without any reference to DateTime objects.

@karstennilsen
Copy link

I also believe a custom regex is a bit silly because strict comparison of DateTimes should not be default behavior

@janedbal
Copy link

You can compare $date1->getTimestamp() === $date2->getTimestamp() to avoid any ignores and be compliant with strict-rules.

@whataboutpereira
Copy link

You can compare $date1->getTimestamp() === $date2->getTimestamp() to avoid any ignores and be compliant with strict-rules.

Thanks, that might not be a bad idea.

@karstennilsen
Copy link

Timestamps are second based, DateTimes can be more precise

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

No branches or pull requests

5 participants