search

laminas / laminas-diagnostics

A set of components for performing diagnostic tests in PHP applications
https://docs.laminas.dev/laminas-diagnostics/
BSD 3-Clause "New" or "Revised" License
55 stars 32 forks source link

PDOCheck.php extends AbstractCheck #14

Closed voodooism closed 3 years ago

voodooism commented 3 years ago
Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA no

Description

I've noticed that all the checks here extend the AbstractCheck class. As for me, it seems strange that the PDOCheck doesn't. This small bug doesn't allow to set a custom label for PDOCheck.

arueckauer commented 3 years ago

@voodooism Thanks for the PR. Can you elaborate how this is a bug?

samsonasik commented 3 years ago

@arueckauer its already in PR desc : 

This small bug doesn't allow to set a custom label for PDOCheck.

Actually, when extends AbstractCheck in another classes, implements no longer needed as AbstractCheck already implements, but probably that should be in another PR.

samsonasik commented 3 years ago

@voodooism please apply DCO see https://github.com/laminas/laminas-diagnostics/pull/14/checks?check_run_id=1378946763

arueckauer commented 3 years ago

To me it more looks more like a design issue than a bug. The setLabel() should be defined in CheckInterface to allow a manual overwrite.

voodooism commented 3 years ago

@voodooism please apply DCO see https://github.com/laminas/laminas-diagnostics/pull/14/checks?check_run_id=1378946763

Done

samsonasik commented 3 years ago

travis in php 5.6 failure, not sure if that worth fixing, probably add allow_failures for php 5.6?

froschdesign commented 3 years ago

@samsonasik

travis in php 5.6 failure, not sure if that worth fixing, probably add allow_failures for php 5.6?

Not necessary if #15 is completed first.

Ocramius commented 3 years ago

IMO this is an enhancement - moved to 1.8.0 and needs a rebase to see if CI is happy with it.

Ocramius commented 3 years ago

Interestingly, extending AbstractCheck instead of implementing CheckInterface makes the quality of the code worse here, raising new psalm (valid) errors: 

ERROR: PropertyNotSetInConstructor - src/Check/PDOCheck.php:11:7 - Property Laminas\Diagnostics\Check\PDOCheck::$label is not defined in constructor of Laminas\Diagnostics\Check\PDOCheck or in any private or final methods called in the constructor (see https://psalm.dev/074)
class PDOCheck extends AbstractCheck

Therefore, decided not to move on with this, as a clean implementation is preferable in general, over adding artificial inheritance layers.

Closing here.