Problem/Motivation

\Drupal\Core\Authentication\AuthenticationCollectorInterface::getProvider() has documentation to say $provider_id should be a string but sometimes a NULL is used which causes a deprecation for PHP 8.5.

Steps to reproduce

Proposed resolution

1. Add typehint to interface
2. Leave code in \Drupal\Core\Authentication\AuthenticationCollector::getProvider alone so if contrib of custom calls with NULL a deprecation is thrown on PHP 8.5.
3. Fix code so it no longer calls it with NULL.

Remaining tasks

User interface changes

None

Introduced terminology

N/a

API changes

Typehint added to conform to docs to \Drupal\Core\Authentication\AuthenticationCollectorInterface::getProvider - should add a follow-up to add typehint to concrete implementation in 12.0

Data model changes

None

Release notes snippet

N/a

Issue fork drupal-3554714

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:

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
andypost’s picture

Looks ok but not clear why only interface updated, it's already API change in interface so implementation could be updated same time

ref https://git.drupalcode.org/project/drupal/-/merge_requests/13602/diffs?c...

andypost’s picture

Issue tags: +PHP 8.5
alexpott’s picture

@andypost - no adding the typehint to the interface is fine and will not break anything. Adding the typehint to the implementation will break any calls in contrib or custom which pass a NULL. If we leave as is it'll trigger a deprecation in PHP 8.5 telling people to fix their code. In Drupal 12 we can add the typehint and then any code that call it with a never will trigger an exception. That way we get a continuous upgrade path.

smustgrave’s picture

@alexpott with regards to your last comment does that need to happen before 12?

alexpott’s picture

@smustgrave No it needs to happen in 12.x once it is open.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the quick reply! In that case I believe this one should be good to go. If I jumped the gun apologize.

  • catch committed cb8fc951 on 11.3.x
    Issue #3554714 by alexpott, andypost: Add string typehint to \Drupal\...

  • catch committed aa5ede4e on 11.x
    Issue #3554714 by alexpott, andypost: Add string typehint to \Drupal\...

catch’s picture

Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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