[policy, no patch] Document named arguments BC policy

Sounds great 👍 it will help with BC for constructors!

Meanwhile it reminds me about protected/private policy for properties (views still using public a lot)

@andypost The post proposes excluding parameter names from the BC policy. That seems like the opposite of what we want for BC for constructors, because changing the name of a named argument would break BC for a site running PHP 8. Parameter name changes are rare in my experience and done for cosmetic reasons or to work around how hard it is to reorder things in a function signature without BC breaks.

Obviously we can't really take advantage of named arguments in Drupal 9 because we support 7.3 and 7.4, but I don't understand why we'd go so far as to declare that something that clearly causes a BC break that'll fatal isn't part of our BC policy.

In Drupal 10 it would be good to take advantage of the named arguments and save a lot of hassle in how to change the signatures of methods, especially constructions, safely when we add and remove services and the like.

@xjm, It's not clear to me which position you favor. Are you saying that we should mark a BC break when we change a parameter name or the opposite?

Right now, I'm of the opinion that renaming a parameter should NOT constitute a BC break at least until we support PHP 7.4. I don't recollect if the minimum PHP version for Drupal 10 is already decided. If (and only if) the minimum is going to be PHP 8, then we could reconsider this position for Drupal 10 onwards (not earlier versions).

I agree that named arguments will help when we reorder parameters or add a parameter in between the parameters list (essentially reordering). But that only helps if all the callers use named arguments to call the function. AFAIK, there is no way to make it mandatory to use named arguments (sort of like kwargs in Python). Also, if a mandatory argument is added to the parameters, I am not sure how named arguments can help over there.

@hussainweb I'm asking a question, not advocating for anything. I don't see how we have a choice other than to treat parameter names as part of the API, because changing them will break code on PHP 8, just as much as changing a function name would.

This is what we are looking at currently:

• Drupal 9.1 already (mostly) supports PHP 8.0, as well as 7.3 and 7.4.
• Drupal 9.2 will support 7.3, 7.4, and 8.0
• Druapl 9.3 will support 7.3, 7.4, 8.0, and hopefully 8.1
• Drupal 10 will have a minimum PHP version of 8.0 if not higher.

I think the policy we actually need is one to the effect that, In Drupal 9 core and earlier, parameters must not be accessed by name and signature arguments must remain in order, and that the parameter names must not change.

Then for D10 we can decide what we want, but D9 should provide a BC promise that makes sense for all PHP versions it supports.

The goal with BC and declaring what is and is not API has to be to balance the needs of developers using the APIs of Drupal core, with the needs of developers changing the APIs of Drupal core, right? There's a lot of overlap obviously -- most core updates are "using" APIs but the core updates that "change" APIs mostly also are also "using" APIs.

In D9.x, we already know that we cannot delete function parameters or insert parameters, because we support PHP 7.x and there you cannot call functions with named parameters, right? Also, 100% of the API calls within 9.x-compatible core/contrib cannot use named parameters. So I'm with Alex here: I don't think it makes sense to say both "You can't delete/insert parameters" and "You can't change names of parameters" -- that is very restrictive and the worst of all possible worlds, and it doesn't really benefit anyone in 9.x.

For 10.x if the minimum PHP version is at least 8, so people could call APIs using named arguments. However, Symfony's policy (proposed anyway?) is:

Parameter names are not part of the compatibility promise. Using PHP 8's named arguments feature might break your code when upgrading to newer Symfony versions.

Which means that a portion of our (inherited) API will not guarantee backwards compatibility for people who call functions with named arguments.

It seems like if we adopt a different policy that guarantees BC for people using PHP's new named argument feature, then we'll have to clarify that it only applies to our own code in the Drupal namespace, and not to code in the Symfony namespace (not sure about what other projects under /vendor are doing?). That seems very confusing, especially since we have classes that inherit methods from vendors. That would mean that some of the methods on a Drupal namespaced class would support BC for parameter names and some would not. Sounds like a bad developer experience to me!

Symfony's approach seems to be to tell people not to use named arguments in calling code at all - so that essentially from an API point of view it maintains the status quo - as if they'd never been introduced.

I think it'd be completely reasonable for us to do this too: tell people we discourage using named arguments, and from there everything else stays the same. If someone uses named arguments to call our code against advice, then they'll be fine until we change an argument name.

To me this seems like a good policy to start with, since it maintains the status quo as long as no-one uses the language feature - but if they do, we told them what the consequences might be.

The case where named arguments seem most useful - constructors, we already say those are completely outside our API anyway. So... if people start calling constructors with named arguments, they're not worse off than now, and they might be better off (i.e. when we remove an argument from a constructor, or re-order, their code will still work) - but this is something contrib can experiment with, and we can document later if it's actually a good idea. Even if we do that later, the constructor argument names would still be outside of our API, it's just one more way for calling code to hedge against changes.

I've removed:

This means...

that changing a parameter name will become a backward compatibility breaking change as of #PHP8.

(https://twitter.com/SyntaxSeed/status/1328680493617995776)

from the issue summary, since IMO it's misleading - it's only a bc break if calling code is using the language feature to call your method, otherwise nothing changes.

I think we should specifically state that argument names may change and cannot be relied on as an api for PHP 8 right now. I do think this might be worth revisiting in the future but probably not until our active development branch requires a php 8 minimum at the earliest. I can see the potential a few years down the road where we could almost require it as part of the api and then we could move parameters around much easier than we can now, but that would be at least a couple major versions away. For now, we don't want to encourage people to use this, it just removes our flexibility. I could potentially see us, sometime in the D10 or D11 cycle, suggesting that named parameters be used, and by D11 or 12 having that give us more flexibility to adjust the order of some arguments in specific places, but if that's even possible, it's a long way off. I suggest specifically stating that method argument names are not BC right now, and having a bigger discussion over the next year or two as to whether it is something that we could change and leverage in the future.

+1 to #12 - this is a new language feature and we don't usually dive in and use new things straight away, as we have to support older PHP versions. When PHP 8 is the minimum we can revisit this if we want to.

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?

Is there an existing coding standards issue for named arguments? Should we file one?

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.

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.

We will need an exception when #3252386: Use PHP attributes instead of doctrine annotations 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,
  ]
)]

Excluding everything at first, then adding gradual exceptions like #24 seems good, not entirely sure how to present that though.

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 :)

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.

Catch confirmed that we have agreement. Setting to RTBC.

I'll make the followup and update the policy.

The followup is #3348312: [policy, no patch] Add exception for PHP attributes in the named arguments policy.

And I have added the text in the IS to the policy.

Both the follow-up and documentation change look good, I think we can mark this fixed.

Thanks for #3348312: [policy, no patch] Add exception for PHP attributes in the named arguments policy, 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 = []);

@dpi like an opt-in for individual methods? That's worth opening a new issue for yeah.