Problem/Motivation

On #3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter it was suggested making the following change on the Hook attribute, but in initial review phpstan failed. Back then, it was something like OrderBefore|OrderAfter|null and ? doesn't work with multiple types. But now that we've got OrderInterface and that's the only option, the ? works just fine.

Steps to reproduce

Proposed resolution

- public OrderInterface|null $order = NULL,
+ public ?OrderInterface $order = NULL,

Remaining tasks

User interface changes

Introduced terminology

API changes

None, these are equivalent type specifications. This is basically just a code style change.

Data model changes

Release notes snippet

Issue fork drupal-3521612

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Title: [pp-1] Explore type hinding tightening on the hook attribute » Explore type hinting tightening on the hook attribute
Related issues: +#3485896: Hook ordering across OOP, procedural and with extra types i.e replace hook_module_implements_alter

nicxvan opened merge request !12069

donquixote’s picture

donquixote commented
Title: Explore type hinting tightening on the hook attribute » Use ? type shortcut instead of |null on Hook attribute

This is a code style change, fully equivalent to original code (except support for some old php versions), so it does not really tighten anything.
Changing title.

In general I am ok with this change.

dww’s picture

dww
we/he/they
commented
Title: Use ? type shortcut instead of |null on Hook attribute » Use ? type shortcut instead of |null for order param on Hook attribute
Status: Active » Needs review
Related issues: +#3523159: Add order parameter to FormAlter attribute

I made the same suggestion at #3523159: Add order parameter to FormAlter attribute so +1 for me. Will RTBC when the pipeline is green.

x-posted a title change, but thankfully d.o prevented that. 😅 Even more accurate title now. 😂

dww’s picture

dww
we/he/they
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Pipeline is now green. Trivial change. Summary and title are clear.

dww’s picture

dww
we/he/they
commented

Saving credit for everyone.

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Even more clear summary. 😅

  • larowlan committed c7521c33 on 11.x 
    Issue #3521612 by nicxvan, dww, donquixote: Use ? type shortcut instead...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and pushed.
Glad I'm wasn't going mad when I questioned this during review. That issue already had me questioning my 25 years of PHP experience with its use of array_is_list and array_replace, two functions I'd never used before.

larowlan closed merge request !12069

dww’s picture

dww
we/he/they
commented

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.