OndraM / ci-detector

Detect continuous integration environment and get information of current build
MIT License
212 stars 12 forks source link

Allow fromEnvironment to be used form a child class #65

Closed theofidry closed 4 years ago

theofidry commented 4 years ago

Replace self to static which allows late static binding to kick in, useful when inheriting the class.

Also note that not passing Env in the constructor instead of instantiating really makes it hard to extend

OndraM commented 4 years ago

Also note that not passing Env in the constructor instead of instantiating really makes it hard to extend

How so? I'm not sure what do you mean 🤔

theofidry commented 4 years ago

Yeah nevermind that, with the change from self to static this is no longer a problem. You can check https://github.com/infection/infection/pull/1261/files for a few cases, the interesting classes:

This PR [#1261] is why both this PR and #64 where opened

OndraM commented 4 years ago

Oh, nice use-cases!

About this PR - have you seen the phpstan error?

 ------ ------------------------------------------------------- 
  Line   src/CiDetector.php                                     
 ------ ------------------------------------------------------- 
  36     Unsafe usage of new static().                          
         💡 Consider making the class or the constructor final.  
 ------ ------------------------------------------------------- 

There are few options how to solve this, what do you think would be the best for you, as someone who use & extend this library inside other library?

Maybe BC break will be neccessary - we can make the constructor final, which would be BC break. Also making class CiDetector final is BC break. What you suggest in #64 seems reasonable, however it is also BC break :shrug:.

theofidry commented 4 years ago

I think the PHPStan error should be ignored here: yes it is unsafe but the proper fix IMO is #64 which can be done for the next major version

OndraM commented 4 years ago

Agreed, I updated the branch.