Key file not loaded when Stream Wrapper is used for file location before Drupal bootstrap completes

We should not limit this to the public and private schemas. Maybe you should use something like:

    if (StreamWrapperManager::getScheme($file)) {
      $file = \Drupal::service('file_system')->realpath($file);
    }

Hi @angel.h, I have updated your changes in the MR6.
I have raised an MR please review if it is fine then please merge it.

MR: https://git.drupalcode.org/project/key/-/merge_requests/6
Thanks

Thanks for informing @kmakaveev, I am updating your changes in MR. Thanks

MR #6 updated. Thanks

I will note that the use of realPath() is discouraged. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21File%21Fi...

I'm not sure how often a site may want a key file on a remote streamWrapper, however if this change is accepted as-is it could break existing deployments.

At a minimal I believe this may need some work to ensure it does not impact remote streamWrappers.

Adjusting title to capture that this should only an issue before bootstrap completes.

We can use the realpath() method of FileSystem.

I have validated the MR changes with shield module and it work as expected. Hence requesting review.

We can use the realpath() method of FileSystem.

Given the following documentation of the API Method can you provide more justification?

    * The use of this method is discouraged, because it does not work for
     * remote URIs. Except in rare cases, URIs should not be manually resolved.
     *
     * Only use this function if you know that the stream wrapper in the URI uses
     * the local file system, and you need to pass an absolute path to a function
     * that is incompatible with stream URIs.
     *

— FileSystemInterface::realpath()

Does the key module know that a path is a local filesystem? Is this a rare case? If so why? Does the module need to pass an absolute path to a function? If so which function only accepts absolute paths?

Minor suggestion ~~ Please check, moving into NW

Hello @cmlara,

I am currently experiencing an issue when provided file path located on a different server or outside the Drupal project scope, as it is returning an following error. Hence I mentioned that we can use the realpath() method of FileSystem.

Could you please let me know if there's anything I'm missing that would enable me to provide a file path located on a different server or outside the Drupal project scope?

Could you please let me know if there's anything I'm missing that would enable me to provide a file path located on a different server or outside the Drupal project scope?

Using the S3fs module with the file at s3://test.txt would be an example that works key when this patch is not applied. Once this patch enforces realpath I would expect the following to no longer function.

> file_put_contents('s3://test.txt', 'testkey');
= 7

> file_get_contents('s3://test.txt');
= "testkey"

> Drupal::service('key.repository')->getKey('s3key')->getKeyValue()
= "testkey"

Hello @cmlara, updated to work with realpath, stream wrapper and remote urls. Could you please validate with s3 once. Will keep this in needs work status.

I have not yet utilized the s3fs module in Drupal, but I will give it a try. Then accordingly update here.

In !MR 6 Looks like there's a complete new feature has been introduced i.e Provide support for external file url for key. I'm not sure if this should be supported or not. In any case, this should be implemented and discussed separately. We should limit this ticket to fix the bug only.

With the understanding how the public:// and private:// stream wrappers function with S3FS. Will close the MR!6(keeping for future reference) and create new one.

Cleanup completed, requesting review.

Referencing existing issue to address PHPStan related warnings - #3520663: Fix PHPStan warnings.

From MR comments it appears this should be Postponed on #3356052: KeyProviderInterface::getKeyValue() doesn't always return a string as there is question if NULL is an acceptable return type.

It is used in one location in existing code however is not documented in the API to be allowed.

Until a decison is made to update the API new deviations should likely not be added. (Avoid creating more work for others if the API is correct.)

Sure @cmlara, will start work and discussion on #3356052: KeyProviderInterface::getKeyValue() doesn't always return a string.

Based on the ongoing discussions in the referenced issues, will proceed with the exact ask of this issue without introducing new validations.

Requesting review.

Overall, the changes in MR!47 look good to me. I validated the behavior using both private://myfile.key and public://myfile.key paths, and was able to retrieve the key value when using this patch. However, without the patch, the value returns null. Therefore, marking this as RTBC.

Since constructor arguments changed on a public class, I think this deserves a change record.

Draft CR - #3559966: FileKeyProvider now uses dependency injection for FileSystemInterface and improved stream wrapper handling please review.

I think the CR is too lengthy compared with other announcements about new constructor arguments - I assume the LLM went too far here :)

I expected nothing more than
* https://www.drupal.org/node/3404140
* https://www.drupal.org/node/3440755
* https://www.drupal.org/node/3419963

The code looks for me.