[policy, no patch] Document named arguments BC policy

Created on 18 November 2020, about 4 years ago
Updated 16 March 2023, almost 2 years ago

Problem/Motivation

PHP 8 introduces named arguments.

Named arguments can't be declared, this is a language feature that allows you to call code using argument names:

setcookie(
    name: 'test',
    expires: time() + 60 * 60 * 2,
);

Proposed resolution

Follow Symfony's lead and explicit exclude argument names from our backwards compatibility policy β†’

Proposed text
Add the following to @internal β†’ section of the API definition.

# Function arguments
Named arguments should not be considered part of the public API.

Remaining tasks

Decide on additions to policy

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

?

🌱 Plan
Status

Fixed

Version

10.1 ✨

Component
BaseΒ  β†’

Last updated 1 day ago

Created by

πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

Live updates comments and jobs are added and updated live.
  • PHP 8.0

    The issue particularly affects sites running on PHP version 8.0.0 or later.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡ΊπŸ‡Έ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
  • πŸ‡ΊπŸ‡Έ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 almost 2 years ago
  • πŸ‡³πŸ‡Ώ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 almost 2 years ago
  • πŸ‡³πŸ‡Ώ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 almost 2 years ago
  • πŸ‡¬πŸ‡§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.

Production build 0.71.5 2024