Fix two more false positives in EnforceReadonlyPublicPropertyRule

[devfrey]

Thanks for merging my previous patch so quickly!

Two more small fixes to the same rule:

1. Readonly properties cannot be initialized with an explicit default value.

Specifying an explicit default value on readonly properties is not allowed, because a readonly property with a default value is essentially the same as a constant, and thus not particularly useful.

2. Untyped properties cannot be readonly.

The readonly modifier can only be applied to typed properties. A readonly property without type constraints can be created using the Mixed type.

https://www.php.net/manual/en/language.oop5.properties.php#language.oop5.properties.readonly-properties

[janedbal]

Even tho I'm not satisfied with this rule anymore (considering removal upon v5), I think the intention there was to guard public props to be encapsulated. And both those issues are easily fixable on PHP 8.0+ in 99% cases (edited). You can move default value to ctor and you can add native typehint. The only cases where you cannot add typehint are:

• It is resource
• It is mixed/union and you use old PHP 7.4 (the lowest supported version)

I think I want to keep it as-is regarding those two.

[janedbal]

So, if you use PHP 7.4, I suggest disabling the rule. If 8+, you dont need this adjustment.

[devfrey]

Thanks for your feedback and taking the time to review.

I can understand your reasoning, but I think it would still be very useful to keep this rule. It helped me track down properties that should should've been readonly (or their classes should've been readonly). I'm on PHP 8.4 by the way.

The reason I "need" these fixes is due to false positives on properties that extend vendor code or are expected to be defined/used in a certain way. Let me share two concrete examples:

1. Jobs in Laravel can define retries/backoff/etc. using magic properties on job classes:

<?php

namespace App\Jobs;

class ProcessPodcast implements ShouldQueue

{
    public int $tries = 5;
}

Because of the default value this property cannot be readonly. Assigning the value in the constructor is possible, but kind of unconventional.

2. Laravel models can also be configured using properties such as disabling timestamps.

<?php

namespace App\Models; 

use Illuminate\Database\Eloquent\Model;

class Flight extends Model
{

    public $timestamps = false;
}

This property exists on the parent class and is untyped. Adding mixed in the child class is therefore not allowed. Even if this was typed (and not readonly), it would still not be possible to add readonly.

What are your thoughts on these examples?

[janedbal]

I think we can make this mergeable if the default value one would be configurable and would behave the same way as before by default:

enforceReadonlyPublicProperty: structure([
      enabled: bool()
      excludePropertyWithDefaultValue: bool() # defaults to false
])

Ensuring native typehint is present should be done via some other rule (or sniff), so we can probably keep that change.

[devfrey]

@janedbal I've made the changes you suggested. Could you review again?