KeyProviderInterface

Created on 24 April 2023, about 2 years ago

Drupal\key\Plugin\KeyProviderInterface::getKeyValue() currently states that the return type will always be a string, but this is currently not the case as EnvKeyProvider and FileKeyProvider currently return NULL and FALSE in some cases. This could lead to some unexpected notices when implementing these methods, for example 🐛 Deprecated function: strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated Postponed: needs info .

🐛 Bug report
Status

Active

Version

1.0

Component

Code

Created by

🇳🇱Netherlands arkener

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @arkener
  • First commit to issue fork.
  • Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update about 2 years ago
    Not currently mergeable.
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
    last update about 2 years ago
    8 pass
  • Status changed to Needs review about 2 years ago
  • 🇮🇳India Ranjit1032002

    Created MR!11 for the issue mentioned, please review.
    Thank You.

  • 🇦🇺Australia yovince Melbourne

    Thanks, @Ranjit1032002 for the patch. The code looks good to me, and it fixed our issue.

  • First commit to issue fork.
  • Pipeline finished with Failed
    14 days ago
    Total: 162s
    #476561
  • Pipeline finished with Success
    3 days ago
    Total: 205s
    #484389
  • 🇺🇸United States cmlara

    The current MR proposes adding return types to the method that conflict with the interface.

    The alternative here is that the interface is 100% accurate and the methods returning NULL/FALSE are wrong.

    I will note there is no API documented method to indicate an error, it appears the interface was written with the assumption of success always occurring.

  • 🇮🇳India rajeshreeputra Pune

    Added return type to all interfaces to discuss and finalise accordingly. Then will add return type to all methods in classes.

  • Pipeline finished with Failed
    about 15 hours ago
    Total: 232s
    #486435
  • 🇺🇸United States cmlara

    @rajeshreeputra

    That seems significantly out of scope for this issue.

    The latest MR makes the API problem worse.

    Adding type hints and return types this would generally need to be a new major only change as it breaks compatibility for those of us who have built modules implementing these interface.

    Suggest addressing on the feedback from #9, that rather than assuming the API is wrong perhaps the code itself is wrong.

    Consider the fact that most of ecosystem is likely not checking for a NULL return (due to API spec) and allowing it does r make sense. An exception would be better since it should be an abnormal condition.

Production build 0.71.5 2024