BooleanArgumentFlag triggers for a property promoted via a constructor (PHP 8)

[roman-eonx]

• PHPMD version: 2.8.2
• PHP Version: 8.0.10
• Installation type: composer
• Operating System / Distribution & Version: Ubuntu 20.04.3 LTS (Focal Fossa)

Current Behavior

A warning for the BooleanArgumentFlag rule is triggered.

Expected Behavior

No warning.

Steps To Reproduce:

1. Define a class with a boolean property promoted via a constructor.

<?php

class SomeEntity
{
    public function __construct(
        protected string $someStringProp,
        protected bool $someBoolProp,
    ) {
        // ...
    }
}

2. Run PHPMD.

Checks before submitting

• Be sure that there isn't already an issue about this. See: Issues list
• Be sure that there isn't already a pull request about this. See: Pull requests
• I have added every step to reproduce the bug.
• If possible I added relevant code examples.
• This issue is about 1 bug and nothing more.
• The issue has a descriptive title. For example: "JSON rendering failed on Windows for filenames with space".

[kylekatarnls]

I agree the violation could be ignored when it's in __construct() rather it's promoted or not actually.

[kylekatarnls]

I started to work on this but the code above does not trigger a violation, it only triggers if the arguments has a default value such as bool $someBoolProp = false

The property being promoted or typed has actually no influence on the rule.

While bool $someBoolProp sounds fine to me, I'm not sure bool $someBoolProp = false is to be encouraged.

The very presence of the default value (added more often for a new feature over being there by initial design) is still a smell in many cases:

class Cat {
  public function __construct(
    private string $name,
    private bool $isActuallyADog = false,
  ) {}
}

This is typically an appendix meant to make a class to support more cases than initially planned. But in this case, CleanCode rule is relevant to warn about this, the best practice is to make some Animal interface and 2 different classes (they may still share some code using either composition or extending a common parent).

When a bool argument is not a hack for cases like above, they usually don't need a default value:

class Cat {
  public function __construct(
    private string $name,
    private bool $isFluffy,
  ) {}
}

In this case it makes sense to always specify true or false to be explicit when creating new instances.

It's still open to discussion.

[AJenbo]

Yeah it's by design that it triggers if there is a default.

[roman-eonx]

it only triggers if the arguments has a default value such as bool $someBoolProp = false

@kylekatarnls in the case of using promotion in entities, I think it's legal to be able to pass a default value when you want an entity property to be true or false by default when a new entity is instantiated. The BooleanArgumentFlag rule is forcing the best practice to not use boolean arguments for any method. But here we deal with property promotion which makes arguments not only method arguments but class property definitions. And having a property with a default value is not a bad practice or a code smell, right?

[AJenbo]

https://phpmd.org/rules/cleancode.html

A boolean flag argument is a reliable indicator for a violation of the Single Responsibility Principle (SRP). You can fix this problem by extracting the logic in the boolean flag into its own class or method.

It is to encourage:

$a = new Entiry();
$b = new ActiveEntity();

Instead of:

$a = new Entity();
$b = new Entity(true);

This has nothing to do with them being properties, the constructor, or property promotion. It would fail just as much for a regular argument that is used to set a property by default.

class SomeEntity {
    public function __construct(bool $someBoolProp = false) {}
}

It is just as much an issue as:

class SomeEntity {
    public function setProperty(bool $someBoolProp = false) {}
}

Solution 1:

class SomeEntity {
    protected $someBoolProp = false;
}
class ActiveEntity extends SomeEntity {
    public function __construct() { $this->someBoolProp = true; }
}

Solution 2:

class SomeEntity {
    public function __construct(protected bool $someBoolProp) {}
}

[kylekatarnls]

And having a property with a default value is not a bad practice or a code smell, right?

That's the point of the Cat isDog vs. isFluffy example above.

• If the boolean changes the meaning/role of the class => it's a code smell as it's indeed violating the principle of single responsibility
• If the boolean is actually a property of your class => why would we have a default value for it? Is new Cat() fluffy? we don't know if we don't enter the code, so it's some arbitrary knowledge that developers has to keep in mind, over specific/arbitrary default, I would recommend to always specify new Cat(fluffy: true) and new Cat(fluffy: false) and if it is neutral, it's very likely not to appear statically in your code but rather to be: new Cat(fluffy: $infoFromOtherSource->hairSize > $threshold) if it's always hardcoded and change significantly the behavior of the entity, then we fall again in the first case, PSR is violated.

[roman-eonx]

@AJenbo "Solution 1" is exactly the issue illustration: the BooleanArgumentFlag rule forces developers to not use promoted properties when an entity has a boolean property that needs a default value. This is not what this rule was introduced for, I believe.

[kylekatarnls]

when an entity has a boolean property that needs a default value

Please show a real example of this "need". I am actually tempted to believe that we have only 2 categories: 1 is breaking the PSR, the other actually should better not have default value, but maybe there is a third one I didn't identify.

[roman-eonx]

My main concern here is that the rule in the topic should not force anything other than passing a boolean argument to a method. It indirectly forces us to avoid default values for an entity boolean property OR to avoid promoted properties. If you think a default value for a property is not a good practice, then there should be a separate rule for this. And one can enable it if one thinks it fits the needs.

As an example to oppose on ActiveEntity extends SomeEntity, you can think of an entity having many boolean properties. E.g. something like feature switches that should be on/off by default when a new entity is instantiated (because this entity knows what values should be by default, in case we decide this entity is responsible for this and not the code instantiating it). To workaround the current rule behavior we have to do one of the following:

• Don't use promoted properties for this entity.
• Don't use default values and always do new SomeEntity(true, true, true, true, true) instead of just new SomeEntity(). Which moves the decision on what default switch values should be out of the entity.
• Use a static constructor method that defines default values.
• Disable the rule.

Do you see? The rule indirectly does what it shouldn't. In other words, it forces you to do something it's not about.

PHPMD has a set of rules that can be combined based on project needs. Developers can enable some rules and ignore the ones they don't agree with or don't need atm. But here the problem is if we think our entities CAN have default values for boolean properties (for whatever reason), we have no way to keep using the rule for avoiding boolean arguments in usual methods (not constructors promoting properties). It's not configurable and it's not smart enough to distinguish the different cases. And we have to disable it because of no other options not affecting the design.

So if you think this rule should still force not using promoted boolean properties with default values, let's maybe consider this behavior to become configurable?

[kylekatarnls]

Just to put this out of the way, it has nothing to do with promoted properties:

class SomeEntity
{
    protected bool $someBoolProp;

    public function __construct($someBoolProp = true)
    {
        $this->someBoolProp = $someBoolProp;
    }
}

Should also raise the violation in the current state.

And we don't raise violation for protected bool $someBoolProp = true; so we don't disallow default boolean value for properties or anything about promoted properties, just about method signature and even if special __construct is still a method. So I don't think the rule is out of scope when checking __construct too.

Having here an ignore-pattern like we have in some other rules would be OK I think, it would allow project to add __construct or whatever method name considered exempted.

[AJenbo]

@AJenbo "Solution 1" is exactly the issue illustration: the BooleanArgumentFlag rule forces developers to not use promoted properties when an entity has a boolean property that needs a default value. This is not what this rule was introduced for, I believe.

Promoted properties lets you define both a constructor argument and an associated property in a single line. Setting a default sets the default for the argument NOT the property. BooleanArgumentFlag does not allow for boolean arguments with default values.

When setting property during the constructor a default for the property is mostly pointless:

class SomeEntity
{
    protected bool $someBoolProp = true;

    public function __construct($someBoolProp)
    {
        $this->someBoolProp = $someBoolProp;
    }
}

(this is not what promoted properties does)

Don't use default values and always do new SomeEntity(true, true, true, true, true) instead of just new SomeEntity(). Which moves the decision on what default switch values should be out of the entity.

That is exactly the point of the rule.

The example here also illustrates two issues with this way of designing things:

• There is no way for me to set argument 4 to false and lave all other as the default. Instead i have to explicitly set 1-3 as true, only 5 can be left as default.
• It's hard for the reader to understand what consequence the booleans have, and easy to confuse them for each other.

I would suggest providing setters for each property instead. It makes the behavior clear, and allows you set individual ones with out forcing you to set others that you do not care about.

[roman-eonx]

Thanks, @kylekatarnls.