Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid having optional parameters before required in method signatures #213

Closed
aik099 opened this issue Dec 27, 2024 · 1 comment · Fixed by #226
Closed

Avoid having optional parameters before required in method signatures #213

aik099 opened this issue Dec 27, 2024 · 1 comment · Fixed by #226

Comments

@aik099
Copy link
Member

aik099 commented Dec 27, 2024

Currently, I'm getting a bunch of deprecations about methods having optional parameters declared before required ones ( see https://github.com/console-helpers/jira-api-restclient/actions/runs/12379854881/job/34554938237 ).

Related to ClientInterface itself and classes implementing it

  1. PHP Deprecated: Optional parameter $data declared before required parameter $credential is implicitly treated as a required parameter in .../src/Jira/Api/Client/ClientInterface.php on line 53
// ClientInterface interface
public function sendRequest(
		$method,
		$url,
		$data = array(),
		$endpoint,
		AuthenticationInterface $credential,
		$is_file = false,
		$debug = false
	);
  1. PHP Deprecated: Optional parameter $data declared before required parameter $credential is implicitly treated as a required parameter in .../src/Jira/Api/Client/CurlClient.php on line 62
// CurlClient class
public function sendRequest(
		$method,
		$url,
		$data = array(),
		$endpoint,
		AuthenticationInterface $credential,
		$is_file = false,
		$debug = false
	) {
  1. PHP Deprecated: Optional parameter $data declared before required parameter $credential is implicitly treated as a required parameter in .../src/Jira/Api/Client/PHPClient.php on line 95
// PHPClient class
	public function sendRequest(
		$method,
		$url,
		$data = array(),
		$endpoint,
		AuthenticationInterface $credential,
		$is_file = false,
		$debug = false
	) {

Related to the Api class

  1. PHP Deprecated: Optional parameter $method declared before required parameter $url is implicitly treated as a required parameter in .../src/Jira/Api.php on line 711
// Api class
public function api(
		$method = self::REQUEST_GET,
		$url,
		$data = array(),
		$return_as_array = false,
		$is_file = false,
		$debug = false
	) {

The @royarisse suggested some interesting ideas (see #205) about fixing:

  1. remove the default value for the ClientInterface::sendRequest method's $data parameter
  2. remove the default value for the Api::api method's $method parameter

I completely agree with 1st item (interface change) and I dare to remove default values from all parameters in the ClientInterface::sendRequest method signature, because:

  1. when the Api class calls the class implementing the ClientInterface interface it will provide all parameters in each call
  2. we're breaking BC anyway with the 2.0.0 release

I also agree with 2nd item, because the current library users (who use the master branch) are forced to specify the request method anyway.

@royarisse
Copy link

As @aik099 stated, even though these changes are good for getting it all working in >=8.1, it breaks <8.1.
I see the only solution would be a (dirty imo) compat loader.

This requires a change in composer.json, something like:

  "scripts": {
    "post-install-cmd": [
      "Composer\\PhpCompat::loadFiles"
    ]
}

And a simple PHP script scr/Composer/PhpCompat.php:

class PhpCompat {
  public static function loadFiles(Event $event) {
    $autoload_file = 'src/Jira/Issues/Walker.php';
    if (php_version_compare(PHP_VERSION, '8.1', '<')) {
      $autoload_file = 'src/Jira/Issues/WalkerCompat.php';
  }

  // ... something
}

I currently don't have the time to complete this, but I think this is the only way for now, as the signatures for Iterator and Countable are changed in PHP>=8.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants