Phpinsights: [Laravel Preset] Job classes use public properties

[deleugpn]

| Q | A
| ---------------- | -----
| Bug report? | no
| Feature request? | no
| Library version | latest

Like the Forbidden Global Constants

ForbiddenDefineGlobalConstants::class => [
    'ignore' => ['LARAVEL_START'],
],

I would like to be able to ignore certain public properties.

• [Code] Forbidden public property:
  app/Synchronizer/Pull.php:18: Do not use public properties. Use method access instead.

Laravel allows us to specify a Queue Connection as a public property on the job itself (https://github.com/laravel/docs/pull/5200) or even the amount of times we should retry the job (https://laravel.com/docs/5.6/queues#max-job-attempts-and-timeout), which conflicts with this Forbidden public property rule.

For now I'm only opening an issue because I don't have enough knowledge to send a PR for this. I'm still getting to know the process, Sniffers, PHP CS, etc.

[dinhquochan]

Same with public $timestamps = false; in Model? Should be ignore public properties forbiren?

[davi5e]

@dinhquochan it's actually more than only $timestamps. From what I could gather, it's $incrementing, $exists, $wasRecentlyCreated, and $timestamps.

As for myself, it'd be nice to at least disable ForbiddenPublicPropertySniff for each of those lines, but I don't know how to do that yet...

[dinhquochan]

So, I think should be removed ForbiddenPublicPropertySniff in Laravel Preset. For now, I do it myself.

cc @nunomaduro

[olivernybroe]

So to fix this I see 3 ways.

1. The most simple way is to just disable the sniff in laravel preset. However this means we now allow all public properties and not only the once we want to.

2. Create our own sniff, which has support for an array of allowed public properties. This will mean we can still disallow public properties, but some of them is allowed. The only issue with this approach is that it allows us to add for example exists property on classes which are not models.

3. Custom sniff with a more advanced settings. So in this case the settings would be something like this

'allowedClasses' => [
    'Illuminate\Database\Eloquent\Model' => [
        'timestamps',
        'exists',
        ...,
    ]
]

This will mean that all classes of the class type (including classes which extends it) will have the listed properties allowed.

I would prefer option 3, even though it adds more complexity, but it also adds flexibility. What are your guys thoughts on this? And do you see any issues with my approach?

[olivernybroe]

So for now you can fix this by using the exclude parameter on the sniff in the config.
https://phpinsights.com/insights/#configure-insights-2

[robsontenorio]

@olivernybroe Once we know Laravel has mandatory public properties, would be useful at laravel preset config file a example for ignoring 'timestamp', for instance.

[olivernybroe]

@robsontenorio I totally agree.

I would like to have the advantage to ignore public properties based on the parent class.
However some issues with this is that phpcs only analyses the current file, so it can see the parent class, but not the parent parent class.

[Gummibeer]

@olivernybroe but the exclude only allows exact file paths? Or could we also use some kind of glob to exclude all files inside app/Models or app/Mails for example?

[olivernybroe]

@Gummibeer yep, we only allow file paths at the moment. We have an issue open for folders #243