- πΊπΈUnited States xjm
It's probably time to revisit this now that D10 is in production with a PHP 8.1 minimum requirement.
In D10, maybe named arguments would fall into the "internal API" category under our policy. They're not public API, but we still should provide a best effort to be non-disruptive in minor releases, as per the Internal API deprecation policy β and the BC policy β .
That said. That's for now. But the outstanding question is: Should we adopt named arguments in core, say as a target for D11?
- πΊπΈUnited States xjm
Is there an existing coding standards issue for named arguments? Should we file one?
- π¬π§United Kingdom catch
I'm still in roughly the same place as #10. We can't enforce anything about what calling code does or doesn't do with this, all we can do is document expectations.
1. I think we should explictly document that we don't support using named arguments for Drupal code (i.e. parameter names are internal).
2. Constructor arguments are already internal, but are probably the place where calling code using named arguments would result in being more resilient against core changes - because then you're not affected by re-ordering. But... it's up to people to try this out if they want and see whether it makes their lives easier or harder.
I don't think we can do anything more than the current bc efforts for parameter names, i.e. we shouldn't do anything extra to deal with named arguments, because that could stop us from implementing bc for other changes, although that's is covered under 'best effort' since sometimes best effort is we can't do anything, and it'll be hard to know without an example.
Contrib code can still use named arguments for PHP functions if they want, they're just on their own if they use it to call Drupal code, so a coding standards issue makes sense just to formalize formatting rather than where to use them or not.
On using them in core, the only place really would be calling code from other projects (phpunit? php itself), it doesn't hurt to have an issue open but I don't see it having many use-cases for us.
- π¬π§United Kingdom alexpott πͺπΊπ
I don't think we should adopt named arguments in core as rule. I think we should chose to adopt it where it is useful and not where it would be a hinderance. We have way too much legacy to freeze all the argument names in core as an API.
- π³πΏNew Zealand quietone
There is now a issue in coding standards, #3337834: Standards for PHP named arguments β .
I also agree with not using named arguments for reasons already stated. For discussion I added the following to the IS as proposed text.
# Function arguments Named arguments should not be considered part of the public API.
- π¬π§United Kingdom longwave UK
We will need an exception when π Use PHP attributes instead of doctrine annotations Fixed lands, as named arguments in attributes are part of the API, by design.
The proposed Block attribute constructor is:
public function __construct( public readonly string $id, public readonly ?TranslatableMarkup $admin_label = NULL, public readonly ?TranslatableMarkup $category = NULL, public readonly array $context_definitions = [], public readonly ?string $deriver = NULL, public readonly array $forms = [] ) {}
This is then invoked with named arguments:
#[Block( id: "page_title_block", admin_label: new TranslatableMarkup("Page title"), forms: [ 'settings_tray' => FALSE, ] )]
- π¬π§United Kingdom catch
Excluding everything at first, then adding gradual exceptions like #24 seems good, not entirely sure how to present that though.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Just for posterity, I agree with excluding these from our API, but I think we should also strive not to break things where possible, like we do with e.g. plugins constructors.
So if we can do it nicely, we should. E.g if we can juggle the old param to the new name, we should, and emit a deprecation error. But to be clear, I don't think we should require this. There will be cases where the old param has no meaning in the new signature.
Re #24 I agree, with #25, but yes, communicating the exceptions will be hard. Perhaps we can have an Attribute for that, attributes on our attributes :)
- Status changed to Needs review
over 1 year ago 12:01am 15 March 2023 - π³πΏNew Zealand quietone
It looks like there is agreement here with the proposal in the Issue Summary. If that is true, then once the policy is updated an issue can be opened for the exception mentioned in #24.
- Status changed to RTBC
over 1 year ago 10:01pm 15 March 2023 - π³πΏNew Zealand quietone
Catch confirmed that we have agreement. Setting to RTBC.
I'll make the followup and update the policy.
- π³πΏNew Zealand quietone
The followup is π± [policy, no patch] Add exception for PHP attributes in the named arguments policy Active .
And I have added the text in the IS to the policy β .
- Status changed to Fixed
over 1 year ago 8:57am 16 March 2023 - π¬π§United Kingdom catch
Both the follow-up and documentation change look good, I think we can mark this fixed.
- π¦πΊAustralia dpi Perth, Australia
Thanks for π± [policy, no patch] Add exception for PHP attributes in the named arguments policy Active , making named args a part of the attribute API makes sense.
Excluding everything at first, then adding gradual exceptions like #24 seems good, not entirely sure how to present that though.
@catch
Is this something that could be represented as documentation or custom PHPdoc annotation for example?
Taking an example of named arguments already used in private code im familiar with in toUrl/toLink methods:
$node->toUrl(options: ['absolute' => TRUE, 'fragment' => $fragment])->toString(); $node->toLink(rel: 'edit-form');
Documentation or annotation such as:
[...] * @return \Drupal\Core\Link * A Link to the entity. * * @throws \Drupal\Core\Entity\EntityMalformedException * @throws \Drupal\Core\Entity\Exception\UndefinedLinkTemplateException * * @namedArguments */ public function toLink($text = NULL, $rel = 'canonical', array $options = []);
- π¬π§United Kingdom catch
@dpi like an opt-in for individual methods? That's worth opening a new issue for yeah.
Automatically closed - issue fixed for 2 weeks with no activity.