Fix up signature for PHP 8+

[dereuromark]

Solves https://github.com/chobie/jira-api-restclient/issues/199

[dereuromark]

@aik099 I recommend changing the github config for first time contributors to allow running CI :) The default unfortunately is not the "new people" only, but everyone on github.

[dereuromark]

I also added PHPStan which was definitly needed, quite a few issues that have been detected, including the PHP 8+ issues:

 ------ ----------------------------------------------------------------------------------------- 
  Line   Jira/Api/Client/ClientInterface.php                                                      
 ------ ----------------------------------------------------------------------------------------- 
  57     Deprecated in PHP 8.0: Required parameter $endpoint follows optional parameter $data.    
  58     Deprecated in PHP 8.0: Required parameter $credential follows optional parameter $data.  
 ------ ----------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------- 
  Line   Jira/Api/Client/CurlClient.php                                                           
 ------ ----------------------------------------------------------------------------------------- 
  66     Deprecated in PHP 8.0: Required parameter $endpoint follows optional parameter $data.    
  67     Deprecated in PHP 8.0: Required parameter $credential follows optional parameter $data.  
 ------ ----------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------- 
  Line   Jira/Api/Client/MemcacheProxyClient.php                                                  
 ------ ----------------------------------------------------------------------------------------- 
  79     Deprecated in PHP 8.0: Required parameter $endpoint follows optional parameter $data.    
  80     Deprecated in PHP 8.0: Required parameter $credential follows optional parameter $data.  
 ------ ----------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------- 
  Line   Jira/Api/Client/PHPClient.php                                                            
 ------ ----------------------------------------------------------------------------------------- 
  99     Deprecated in PHP 8.0: Required parameter $endpoint follows optional parameter $data.    
  100    Deprecated in PHP 8.0: Required parameter $credential follows optional parameter $data.  
 ------ ----------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   Jira/Issues/Walker.php                                                                                                                             
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------- 
  152    Return type mixed of method chobie\Jira\Issues\Walker::current() is not covariant with tentative return type mixed of method Iterator::current().  
         💡 Make it covariant, or use the #[\ReturnTypeWillChange] attribute to temporarily suppress the error.                                             
  171    Return type mixed of method chobie\Jira\Issues\Walker::next() is not covariant with tentative return type void of method Iterator::next().         
         💡 Make it covariant, or use the #[\ReturnTypeWillChange] attribute to temporarily suppress the error.                                             
  182    Return type mixed of method chobie\Jira\Issues\Walker::key() is not covariant with tentative return type mixed of method Iterator::key().          
         💡 Make it covariant, or use the #[\ReturnTypeWillChange] attribute to temporarily suppress the error.                                             
  201    Return type mixed of method chobie\Jira\Issues\Walker::valid() is not covariant with tentative return type bool of method Iterator::valid().       
         💡 Make it covariant, or use the #[\ReturnTypeWillChange] attribute to temporarily suppress the error.                                             
  263    Return type mixed of method chobie\Jira\Issues\Walker::rewind() is not covariant with tentative return type void of method Iterator::rewind().     
         💡 Make it covariant, or use the #[\ReturnTypeWillChange] attribute to temporarily suppress the error.                                             
  280    Return type mixed of method chobie\Jira\Issues\Walker::count() is not covariant with tentative return type int of method Countable::count().       
         💡 Make it covariant, or use the #[\ReturnTypeWillChange] attribute to temporarily suppress the error.                                             
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------- 

[dereuromark]

Some other issues I found that are not optional:

• Deprecations around PHPUnit\Framework\TestCase::prophesize() is deprecated and will be removed in PHPUnit 10. Please use the trait provided by phpspec/prophecy-phpunit.
• composer.lock file is commited, which it should not be, instead the upper and lower limit should be tested
• PHP 5.4 at this point is definitely not warranted anymore, maybe upgrade to 7.3 or better 7.4 now.
• it also suffices to only test 7.4 + 8.2 for example (min/max)

[dereuromark]

PHPStan isnt easily runnable on PHP < 7.1, I would recommend dropping those versions as as written above, since those are long EOL anyway.

[dereuromark]

Regarding saving time: It would save both our time if the github config was to be adjusted to the recommended setting.

[dereuromark]

Refs https://github.com/Riverline/jira-api-restclient/pull/1

[dereuromark]

I removed PHP8.2 again, this must be checked separately it seems, since here we need --ignore-platform-req=php still I can do that as a smoke test in parallel. Just to make sure nothing is missing for future compatibility.

Also added some shiny badges that outline the repo infos a bit more clearly on the top As I do for my repos as well ( e.g. https://github.com/dereuromark/cakephp-ide-helper ).

[aik099]

PHPStan isnt easily runnable on PHP < 7.1, I would recommend dropping those versions as as written above, since those are long EOL anyway.

That's why I'd recommend either not using it or using it on the supported (by PHPStan) versions of the PHP.

Regarding saving time: It would save both our time if the github config was to be adjusted to the recommended setting.

I have no idea why GitHub is asking me to approve every commit you make on this PR. If you have any idea why then please share it with me.

Anyway since a typo PR of yours was now merged I'd expect GitHub to treat you as a non-first-time contributor and will auto-run builds on your commits.

I removed PHP8.2 again, this must be checked separately it seems, since here we need --ignore-platform-req=php still I can do that as a smoke test in parallel. Just to make sure nothing is missing for future compatibility.

The need for --ignore-platform-req=php has arrised only after you've locked the PHP version in the composer.json. As I've stated in my latest review this isn't a good move.

Also added some shiny badges that outline the repo infos a bit more clearly on the top As I do for my repos as well ( e.g. https://github.com/dereuromark/cakephp-ide-helper ).

Thank you.

[dereuromark]

    public function testSetDelegateError()
    {
        $this->expectException('\Exception');
        $this->expectExceptionMessage('passed argument is not callable');

        $walker = $this->createWalker();
        $walker->setDelegate('not a callable');
    }

Adding a typehint is more radical than just fixing the documentation. I am not a fan of that, it would also break above test (which then we need to remove).

[dereuromark]

True, the lock file should be completely removed, as I showed in the PHP8.2 check but I reverted the version locking for now.

[aik099]

  public function testSetDelegateError()
  {
      $this->expectException('\Exception');
      $this->expectExceptionMessage('passed argument is not callable');

      $walker = $this->createWalker();
      $walker->setDelegate('not a callable');
  }

Adding a typehint is more radical than just fixing the documentation. I am not a fan of that, it would also break above test (which then we need to remove).

It's fine. You can drop the test then or adjust it for a new error message. The end result for the user would be the same: an exception with one text or another.

True, the lock file should be completely removed, as I showed in the PHP8.2 check but I reverted the version locking for now.

You're not following, are you? No dropping supported PHP versions. No removing composer.lock.

[dereuromark]

If you want this PR to be merged I recommend listening to and discussing the suggestions I'm doing in this review.

I want to be clear to you here. I am putting my work + free time here into getting your project maintained and on a quality level again. It was clearly not very maintained so far. This is blocking us now on a critical timeframe. If you are not interested in an actual collaboration and moving forward here, I will have to fork and create own maintenance on this within the next 48h to unblock us. I am not interested in a long discussion on some of the critical key components, including proper stan check and up to date test matrix. I have been using those for years on 1000+ repos and I do know when I can trust them. It would be wise for you to do the same. PHP 8.2 shows that here are even hidden "gotchas" like non sensical null checks, and even property assignment issues ($subTask vs $subtask) that will blow up in PHP 8.2. On other issues you saw that I am open for discussion, where it makes sense. I do need to push back however on reverting some important improvements.

As for versions: Feel free to add again whats possible, but it is clearly not needed and shouldnt be < 7.3 ever at this point. I dont want to spend time here to try to make things maintainble going that far back while keeping PHPStan support on.

So again: We need to move forward here with both our times not wasted much more. Please let me know if you prefer to have it maintained collaboratively - or want me to start maintaining a fork.

Apart from this I recommend you to try to

• update PHPStan to recommended level 7/8 - 8 is nullable safety which is of course nice to have
• remove lock file as this is a clear anti pattern in library dev

[aik099]

Thanks for bringing some clarity on your motivation here.

My vision is: do the minimal changes, that are necessary to make things happen. That includes:

• if it works - don't change it
• whoever needs a feature added I'm up to it (as a reviewer) as long as we agree on the implementation
• bugfixes are always welcome
• new PHP version compatibility - why not, but not dropping support for other PHP versions (I don't care if they're not used by famous people in the community)

Time is a precious resource, and I don't want to spend it on things nobody needs.

To summarize: if you're in a hurry, then please do what you see fit in your fork. When you have time, then please come back with PRs you are willing to share.

Thank you.