Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #210 +/- ##
============================================
+ Coverage 87.38% 87.41% +0.02%
- Complexity 1188 1196 +8
============================================
Files 60 60
Lines 3837 3862 +25
============================================
+ Hits 3353 3376 +23
- Misses 484 486 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
Looks great! Though this prompts for a slight refactoring of ProviderMetadata constructor.
| ?string $credentialsUrl = null, | ||
| ?RequestAuthenticationMethod $authenticationMethod = null | ||
| ?RequestAuthenticationMethod $authenticationMethod = null, | ||
| ?string $description = null |
There was a problem hiding this comment.
This is getting out of hand in terms of parameters :)
Can we change this to a single param? Either some value object with these, or just the basic $args pattern. Don't love the latter, but would be fine - another class for this seems overkill. I think the first 3 parameters should remain as-is, but the optional ones should be combined.
We can make it backward compatible by supporting either an array for the 4th parameter, or what we currently have for the 4th and 5th parameter. And use a trigger_error for deprecation warning if the old way is used.
Then 2-3 releases later we can remove the old way.
Please make sure to keep tests around for both behaviors to have BC covered but also the new correct way.
There was a problem hiding this comment.
Hahah! Yeah... what we really want here is PHP 8.0 named parameters. In the meantime, I think I'd prefer $args. I don't love it, but I find VO instantiation to be even more awkward, especially for a DTO like this. It makes the parameters feel weirdly complicated.
So how about this as the new signature: __construct(string $id, string $name, array $args = [])
There was a problem hiding this comment.
Works for me, I think. So that means we'll have a default type of Cloud provider then, right?
There was a problem hiding this comment.
Oops! Missed that... I mean, yeah, default cloud provider. That's why I left that out... 😁
There was a problem hiding this comment.
@felixarntz Tentatively resolved in b2228bd
I'm honestly not convinced that I like this better. It's awkward having a growing list of parameters, for sure, but if we eventually bump to 8.0 I'd actually want to go back to pure parameters as it is now. Types and stuff get really goofy. It feels like we're temporarily weakening types to make the DX slightly better. I'm tempted to revert this and just live with it until named parameters are an option.
There was a problem hiding this comment.
Hmm, fair point. Current DX is IMO worse without the array (functions with more than 2-3 params are generally a problematic pattern for DX). Though you can also argue against the array of $args because a lack of auto-completion, and less idiomatic for future PHP compat indeed.
Let's revert - you have me convinced :)
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
Almost good to go! Preemptively approving, but please address the remaining feedback before merge.
| } | ||
| if (isset($allArgs[5])) { | ||
| $args[self::KEY_DESCRIPTION] = $allArgs[5]; | ||
| } |
There was a problem hiding this comment.
This last one is unnecessary. It was never possible in the old way to pass a description anyway. We should not add support for it in the wrong way that we want to discourage anyway.
| /** | ||
| * Gets the provider's description. | ||
| * | ||
| * @since 0.5.0 |
There was a problem hiding this comment.
n.e.x.t - please double check that throughout the PR
2 participants
This adds "description" as an optional field for providers. This is useful in contexts where providers are being listed out for users.