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 #3298726: Deprecated function: strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated.

CommentFileSizeAuthor
#11 key_3356052_11.patch1.56 KByovince

Issue fork key-3356052

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

Arkener created an issue. See original summary.

arkener’s picture

arkener commented
Title: KeyProviderInterface » KeyProviderInterface::getKeyValue() doesn't always return a string

Ranjit1032002 made their first commit to this issue’s fork.

Ranjit1032002 opened merge request !11

ranjit1032002’s picture

ranjit1032002
Primary language English
Location India
commented
Status: Active » Needs review

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

yovince’s picture

yovince
Location Melbourne
commented

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

rajeshreeputra made their first commit to this issue’s fork.

cmlara’s picture

cmlara
he/him
commented

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.

rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented

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

cmlara’s picture

cmlara
he/him
commented

@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.

yovince’s picture

yovince
Location Melbourne
commented
StatusFileSize
new key_3356052_11.patch1.56 KB

The later commits made things more complicated... I'm uploading a patch just for use in my project. Thanks.

rajeshreeputra’s picture

rajeshreeputra
he/him
Location Pune
commented

Agree with you that this will introduce compatibility issues and should be implemented only in a new major version. The commit has been removed.

mxr576 made their first commit to this issue’s fork.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

Needs review ++

ptmkenny’s picture

ptmkenny commented

Working on the Key Per User module, I came across this issue today. Key Per User uses Sodium encryption keys for each user account. Previously, I used an empty string for the anonymous user account, but because of #3608554: Validate key length when checking dependencies (my commit), Sodium module now validates key length, so I used NULL instead of the empty string, but that causes tests to fail because the interface is marked as returning string, even though some of the providers in the module also return NULL.

So, my view is that instead of forcing NULL to be an empty string, we should go the other way and change the interface to allow NULL as well. If we force everything to be a string, we can't do key length validation and allow empty/null values.