PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule is not

[VincentLanglet]

Configuration was introduced for rules in #182 (cc @MartinMystikJonas)
But not for PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule

Is there a reason ?

When using bleedingEdge (for other phpstan feature) it would be nice to still be able to disable the DisallowedLooseComparisonRule.

Should we create a new config parameter which default to %featureToggles.bleedingEdge% ?

[MartinMystikJonas]

I did not touched it because it was already configurable. But I think it is good idea to add it condigurable and defaulted to bleeding edge. @ondrejmirtes Do you agree? I will make PR

[VincentLanglet]

So far all the PHPStan\Rules\DisallowedConstructs rules were under strictRules.disallowedConstructs

Here I wonder if we will have an issue since if we add it under the same config, it will be a bc break
but if we don't, we will loose consistency.

We might also decide to split the strictRules.disallowedConstructs config.

For instance, DisallowedEmptyRule and DisallowedShortTernaryRule are rules which requires a lot of changes in a legacy code base. The DisallowedLooseComparisonRule too. And if I want to disable one of them, I have to disable such easier rules like DisallowedImplicitArrayCreationRule which is kinda sad.

[MartinMystikJonas]

I would preffer to keep BC and just add strictRules.disallowedLooseComparsion.

You can disable strictRules.disallowedConstructs and then add handpicked rules using:

rules:
  - PHPStan\Rules\DisallowedConstructs\DisallowedImplicitArrayCreationRule

[ondrejmirtes]

I think it's a bit tricky to set up the configuration correctly.

So we want to have a new option to turn on/off DisallowedLooseComparisonRule comfortably. It needs to behave this way:

By default:

• if bleedingEdge is not enabled and allRules is enabled, the rule CANNOT be enabled
• if bleedingEdge is enabled and allRules is enabled, the rule MUST be enabled
• if bleedingEdge is not enabled and allRules is not enabled, the rule CANNOT be enabled
• if bleedingEdge is enabled and allRules is not enabled, the rule CANNOT be enabled

With this new option set to true:

• if bleedingEdge is not enabled and allRules is enabled, the rule MUST be enabled
• if bleedingEdge is enabled and allRules is enabled, the rule MUST be enabled
• if bleedingEdge is not enabled and allRules is not enabled, the rule MUST be enabled
• if bleedingEdge is enabled and allRules is not enabled, the rule MUST be enabled

With this new option set to false:

• if bleedingEdge is not enabled and allRules is enabled, the rule CANNOT be enabled
• if bleedingEdge is enabled and allRules is enabled, the rule CANNOT be enabled
• if bleedingEdge is not enabled and allRules is not enabled, the rule CANNOT be enabled
• if bleedingEdge is enabled and allRules is not enabled, the rule CANNOT be enabled

Does this make sense? Can this be achieved in neon config?

[MartinMystikJonas]

@ondrejmirtes I am not sure it is possible in neon. It would require logical operations in config. I will look at it.

[MartinMystikJonas]

@ondrejmirtes Basically we would need a way to do:
DisallowedLooseComparisonRule = strictRule.disallowLooseComparsion ?? (bleedingEdge && strictRules.allRules)
Correct?

[MartinMystikJonas]

It seems impossible in neon. But I have idea. What about make conditonalTags extension accept not only bool but also some struct with advanved conditions? I will think about some resonable format for that

[ondrejmirtes]

Can you imagine doing it with a conditionalIncludes compiler extension?

[MartinMystikJonas]

What about this?

parameters:
  strictRules:
    allRules: true
    disallowedLooseComparsion: [%strictRules.allRules%, %featureToggles.bleedingEdge%]
    # ...

parametersSchema:
  strictRules: structure([
    allRules: bool(),
    disallowedLooseComparsion: anyOf(bool(), arrayOf(bool()))
    # ...
  ])

conditionalTags:
  PHPStan\Rules\DisallowedConstructs\DisallowedLooseComparisonRule:
    phpstan.rules.rule: %strictRules.disallowedLooseComparsion%

And extends ConditionalTagsExtension so it accepts not only bool but also array of bools that all have to be true for tag to be added.

[ondrejmirtes]

I'd like disallowedLooseComparsion: and(%strictRules.allRules%, %featureToggles.bleedingEdge%) if it's possible...

[MartinMystikJonas]

Not sure of it is possible and it definetly would be way more complicated.

[ondrejmirtes]

My issue with [%strictRules.allRules%, %featureToggles.bleedingEdge%] is that it's not obvious whether it's && or ||...

[MartinMystikJonas]

I know it is not ideal but there is no easy way to do it. Problem is that it needs to be evaluated compile-time and nette/di does not support this. So it would (as far as I know) require writing own NeonAdapter and ParametersExtension. Otherwise everything that is not primitive type is converted to Statement and transformed to PHP code evaluated run-time.

Only other option I can think about is [%strictRules.allRules%, '&&' , %featureToggles.bleedingEdge%] and writing own expression evaluation in ConditionalTagsExtension. It can be done (I implemented similar thing for https://github.com/dermatthes/text-template) but it is lots of work and I do not like this format with "random" strings.

[ondrejmirtes]

We already have custom NeonAdapter to expand relative paths based on the directory where the config file is: https://github.com/phpstan/phpstan-src/blob/1.8.x/src/DependencyInjection/NeonAdapter.php

Do you see a way to adapt that?

[MartinMystikJonas]

I need to look at it. It woudl require to detect and() as special type of construct that is transformed to something else than Statement (and I am not sure it would be accepted by later processing in nette/di) and then in ConditionalTagsExtension write code to evaluate these special objects.

[ondrejmirtes]

Alright, but don't spend too much time on it :) We don't need to solve this at all as these rules are gonna have a standard parameter associated in 2.0.

[MartinMystikJonas]

I am checking what could be done. At this point it seems that easiest and best option would be PR to nette/di that will add this possibility there. It would require many changes in many files and maintainig own versions of all these files would be burden for PHPStan.

[MartinMystikJonas]

Or we can just use that option with array and document it somewhere that it is always AND. As far as I can see ConditionalTagsExtension is more or less internal thing anyways.

[ondrejmirtes]

Alright, array it is. Please send the pull requests :)

[MartinMystikJonas]

OK I will prepere PRs

[MartinMystikJonas]

@ondrejmirtes There are no tests for ConditionalTagsExtension? I finished implementation but I wanted to add tests for it and I did not found any existing test for it.

[ondrejmirtes]

Probably not, it's tested by the fact that the configuration works as expected.

You could test it by extending PHPStanTestCase, providing different neon files in overriden getAdditionalConfigFiles(), and inspecting what's in self::getContainer()->getParameters().

[MartinMystikJonas]

Frist PR ready for review here: phpstan/phpstan-src#1697 Then I will make change in strict-rules

[MartinMystikJonas]

We would need phpstan release so we can bump required phpstan version to version with multiple conditions support first.

[herndlm]

The phpstan release is most likely not necessary. After it was merged you should the able to require the next patch version and it will pull the current dev branch :)

[VincentLanglet]

Solved by #191 I think

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.