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

RFC: Support for Options Array and PHP 8.0 Named Parameters #325

Closed
bshaffer opened this issue Mar 5, 2021 · 5 comments
Closed

RFC: Support for Options Array and PHP 8.0 Named Parameters #325

bshaffer opened this issue Mar 5, 2021 · 5 comments
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@bshaffer
Copy link
Contributor

bshaffer commented Mar 5, 2021

RFC for supporting both named parameters in PHP 8 and options arrays for previous versions of PHP

Given class Foo with two "options" in its constructor, string cacheKey and int cacheLifetime, this would give us the following behavior in PHP 8.0:

new Foo(
    cacheKey: 'foo',
    cacheLifetime: 123
);

And the following behavior in PHP 7.4 and below:

new Foo([
    'cacheKey' => 'foo',
    'cacheLifetime' => 123
]);

For this case, the class constructor would look like:

class Foo 
{
    public function __construct(
        array $options = null,
        string $cacheKey = null,
        int $cacheLifetime = 1500
    ) {
        // ...
    }
}

The following cases would throw exceptions:

// provide both options array and named parameters
new Foo(
    ['cacheKey' => 'foo', 'cacheLifetime' => 123],
    cacheKey: 'bar',
    cacheLifetime: 456
);
// provide both options array and ordered parameters
new Foo(
    ['cacheKey' => 'foo', 'cacheLifetime' => 123],
    'bar',
    456
);
// provide options as a named parameter with other named parameters
new Foo(
    options: ['cacheKey' => 'foo', 'cacheLifetime' => 123],
    cacheKey: 'bar',
    cacheLifetime: 456
);
// provide empty options array as a named parameter with other parameters
new Foo(
    options: [],
    cacheKey: 'bar',
    cacheLifetime: 456
);
// provide empty options array as the first argument with other parameters
new Foo(
    [],
    cacheKey: 'bar',
    cacheLifetime: 456
);

It's not possible (AFAICT) to throw exceptions in the following non-standard scenarios:

// provide null options as a named parameter
new Foo(
    options: null,
    cacheKey: 'bar',
    cacheLifetime: 456
);
// provide null options as a named parameter
new Foo(
    null,
    cacheKey: 'bar',
    cacheLifetime: 456
);
// provide options as a named parameter by itself
new Foo(
    options: ['cacheKey' => 'foo', 'cacheLifetime' => 123],
);

This behavior is done by checking if $options is an array and calling func_num_args(). Using a trait, we can guarantee standard behavior across all our classes:

trait OptionsArrayWithAdditionalParametersTrait
{
    private function verifyOptionsArrayWithAdditionalParameters(
        ?array $options,
        int $numArgs
    ) {
        if (is_array($options) && func_num_args() > 1) {
            throw new LogicException(
                'Cannot supply options array with additional parameters'
            );
        }
    }
}
class Foo 
{
    use OptionsArrayWithAdditionalParametersTrait;

    public function __construct(
        array $options = null,
        string $cacheKey = '',
        int $cacheLifetime = 1500
    ) {
        $this->verifyOptionsArrayWithAdditionalParameters(
            $options,
            func_num_args()
        );

        $this->cacheKey = $options['cacheKey'] ?? $cache;
        $this->cacheLifetime = $options['cacheLifetime'] ?? $cacheLifetime;
    }
}

Pros

  • Named parameters can be used in PHP 8
  • This allows for typehinting and cleaner syntax
  • There is still a usable interface for earlier PHP versions.

Cons

  • Some of the possible constructor cases are non-standard
  • this behavior may be considered hacky or strange
  • it allows for classes to be constructed without typehints.
@bshaffer bshaffer changed the title Support for Options Array and PHP 8.0 Named Parameters RFC: Support for Options Array and PHP 8.0 Named Parameters Mar 5, 2021
@Crell
Copy link

Crell commented Mar 5, 2021

Please don't.

All it does is add complications and complexity. Remember that the number of PHP installations that support named arguments (PHP 8.0+) will only ever increase from now on, and those that do not will only ever decrease. You'd be adding extra complexity to your code to support a user base that will only ever shrink, no matter what you do.

Plus, if you're only dealing with 2-4 arguments, it's not a huge burden to just use positional arguments. We've used positional arguments for 25 years now just fine, thank you, and in practice most function/method calls will continue to do so for the foreseeable future. Telling people "call this method the same way that billions of lines of code have done for the last quarter century... or upgrade to PHP 8 if you really need to" is a very reasonable stance to take.

@bshaffer
Copy link
Contributor Author

bshaffer commented Mar 5, 2021

@Crell thank you for weighing in!

While the example only uses two options, this paradigm would be implemented in cases where there are more than 2-4. In #322, the options arrays have 5-7 arguments, and the OAuth2 class has 23 😖 . So we need to have non-positional arguments, and we can do so with an options array, by requiring PHP 8, or by doing this solution here.

@Crell
Copy link

Crell commented Mar 5, 2021

Oauth2 has 23

😱

Unless that's a value object with lots of optional parameters, I'd consider that a code smell on its own that requires refactoring, not clumsy workarounds.

If you must have an options-array-alike, convert it to an object in the first place. At which point you can put a real API on it, and potentially even shift the functionality to that object instead.

cf: https://youtu.be/3mcGzBnSm1c?t=3339

@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Mar 6, 2021
@dwsupplee dwsupplee added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Mar 11, 2021
@MrMage
Copy link

MrMage commented Apr 20, 2021

As a user of these libraries (and the GCloud SDK), I am in favor of supporting Named Parameters for PHP 8 users. It would certainly be easier to understand than an $options array where I need to read the documentation of each individual constructor to figure out the subset of options that this particular constructor understands.

@bshaffer
Copy link
Contributor Author

This is now being discussed here: googleapis/google-cloud-php#5147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

5 participants