@slasher This patch is submitted on the wrong issue. This issue is for ongoing work in progress for the 3.0.x branch, which is not yet ready for use on any site.
If this is a patch for the 2.2.x branch and there is not an existing open issue for that branch, please open one and attach your patch there.
This branch is currently under development and is not intended for use at this time.
I am marking this postponed for now.
Awesome! Thanks for fixing!
@Eli-T: Yes, I have detected no compatibility issues with 2.2.0-alpha3 and drupal 10.3 at this time either.
I had the same question, until I started poking around the module, read the README.md, and looked at the example.style_options.yml file.
This module at its core is build around the Spectrum colorpicker library. That documentation was more helpful than anything, after reading that, the module's README and example.style_options.yml file made alot more sense.
lhridley → created an issue.
lhridley → created an issue.
What happened to the Layout Builder data? Looks like it was removed at some point. Where is the open issue in the issue queue where this is being included. If it is in another contrib module or a submodule, where can that project be found?
I disagree with this for the following reasons:
- When a stream wrapper is registered with the Stream Wrapper Manager service, that stream wrapper is associated with a particular scheme (i.e.,
public://,private://etc.
. Since you can't have two stream wrapper services associated with the same scheme, it only make sense to return a single instance.. This is not unlike other stream wrapper services that are present outside of Drupal, namely:
If anything is wrong, it is more likely to be the documentation.
Updating the link to the Symfony documentation to reflect the current version of symfony in use in Drupal 10.2
@smustgrave, thank you for asking.
1. Clarity on whether the typehint needs to be string|bool vs string|FALSE
2. If the decision is string|FALSE do we also need to revise the return types for LocalStream::getLocalPath() and PublicStream::getLocalPath() to string|FALSE as well.
For clarity, the method ::getLocalPath() in the LocalStream and PublicStream classes are protected methods that are called directly by LocalStream::dirname() and PublicStream::dirname() but have a return type of string|bool instead of string|FALSE.
At least, this is the next step from my vantage point.
@richard_hoogstad if you would be kind enough to open this issue in the flysystem_s3 project, and relate it to this issue, I would very much appreciate that.
@ Watergate → . I'm going to mark this issue as postponed for the moment, I'm in the (hopefully) final stages of refactoring version 3.0 of this module to:
* Utilize League\Flysystem version 3.0
* Drop the current plugin system (since plugins were eliminated in League\Flysystem 2.0) and implement a new plugin system for adapters for version 3.0 (I'm working on that now)
* Implement a Flysystem StreamWrapper that is compliant with Drupal\Core\StreamWrapper\StreamWrapperInterface
* Decorate Drupal\Core\File\Filesystem to allow a seamless switching between file management using Drupal Core and file management using Flysystem
* Allow for configuration of League\Flysystem adapters through the UI
This is a major overhaul that is long overdue.
I'm happy to notify you when the Adapter plugin system is in place if you'd like to modify your PR for version 3.0, as I have not built in any drupal hooks but am open to doing so.
lhridley → created an issue.
Reopening this issue, as it is now relevant to the 3.0.x branch work. Will evaluate to determine what changes may be needed.
@catch The use case would be to open up the use of the StreamWrapperManager for managing stream wrappers that may not necessarily need to be 100% compliant with PhpStreamWrapperInterface.
Right now StreamWrapperManagerInterface defines certain parameters for methods that explicitely expect entities that comply with StreamWrapperInterface. By removing PhpStreamWrapperInterface from the dependency string this opens up the ability to do so.
There are multiple places in Drupal Core where multiple interfaces are implemented. The current entities that implement StreamWrapperInterface as it is defined today would simply need to be modified to also implement PhpStreamWrapperInterface.
All of the methods on StreamWrapperInterface except for two (which IMO should not be there, see the discussion on the related issue) are designed explicitely for registering a StreamWrapper entity with a StreamWrapperManager implementing StreamWrapperManagerInterface. By decoupling StreamWrapperInterface from PhpStreamWrapperInterface you make the StreamWrapper Management component in Drupal Core much more flexible, and would allow for implementing alternative File systems (Like a NoSQL implementation) for Drupal while still taking advantage of the Stream Wrapper Management component.
It's all about decoupling.
Where is the duplicated issue?
Changing description of issue, since interface definition of return types on StreamWrapperInterface::dirname() is no longer relevant, and pursuant to the recently added comment
#32
🐛
StreamWrapperInterface return types for realpath() and dirname() are inconsistent with documented use and intention
Needs work
.
@kim.pepper -- after looking at the return types traced back on the actual implementation of protected methods in LocalStream, I modified it for consistency.
I have no strong feelings about string|bool vs string|FALSE, but changed the original PR for consistency with current implementations. I'm open to modifying this to string|FALSE, but would that not also mean that the return type for LocalStream::getLocalPath() would also need to be updated?
lhridley → changed the visibility of the branch drupal-3271894-3271894-streamwrapperinterface-return-types to hidden.
Circling back for additional information in looking at the implementation of stream wrappers in Drupal core, as it is relevant to this discussion.
Drupal core has three active stream wrappers currently registered:
Two of the three stream wrappers extend the abstract class \Drupal\Core\StreamWrappers\LocalStream (PublicStream and TemporaryStream), and the third, AssetsStream, extends PublicStream.
LocalStream::realpath() returns the result of LocalStream::getLocalPath(), which has return types of string|bool, and in code will return either FALSE, or a string..
PublicStream::getLocalPath() overrides the parent (LocalStream::getLocalPath()) method, and also return either FALSE, or a string.
TemporaryStream by inheritance implements the code defined in it's parent, LocalStream.
In practice, it appears that Drupal Core's stream wrappers fail to adhere to the current Interface as it is described.
In tracing the source of the implementation of StreamWrapperInterface::dirname() back through the same streamwrappers, all inherit from LocalStream::dirname(), which is defined with a return type of string on the interface, and adheres to that.
Conclusion: the definition of StreamWrapperInterface::realpath() should be modified to implement return types of string|bool, as described in the current comments.
This change will not break B/C in core, but should probably be identified in a release as a breaking change, as it could impact contrib modules that are using Drupal\Core\StreamWrapper\StreamWrapperInterface. Also worth noting, none of the stream wrappers in contrib modules that I've reviewed implement ::realpath() in adherance to the Interface, so they should in practice be fine.
lhridley → changed the visibility of the branch 3196064-stub-out-initial to hidden.
lhridley → changed the visibility of the branch 3439327-refactor-the-3.0.x to hidden.
Additional info on contrib modules:
Remote Stream Wrapper
→
, which implements Drupal\Core\StreamWrapper\StreamWrapperInterface by extension. Looks like this module implemented StreamWrapperInterface::realPath() by the documentation and not by the interface defined return type. StreamWrapperInterface::dirname() is implemented by the defined return type, which is string.
::realPath
Returns FALSE;
https://git.drupalcode.org/project/remote_stream_wrapper/-/blob/2.x/src/...
::dirname
Returns a string;
https://git.drupalcode.org/project/remote_stream_wrapper/-/blob/2.x/src/...
@cmlara You summed it up nicely:
It is important to note that no PHP streamWrapper ever implements these methods. See https://www.php.net/manual/en/class.streamwrapper.php
I will admit I always found it a bit clumsy that in Drupal a streamWrapper class implements code intended for use by PHP directly and code that Drupal Core expects to call into directly rather than separating them into two different systems.
The example of how not having the ::dirname() defined on the StreamWrapperInterface is, a module maintainer doesn't have to fight with working out how to handle a method that is totally irrelevant to the functionality of a StreamWrapper, and write and maintain unnecessary code that contributes no value to the functional aspects of the StreamWrapper adapter that is being registered with Drupal's StreamWrapperManager service.
I've been a maintainer of the Flysystem project for about 4 years, I got involved whenone of the maintainers left the community and we had several client projects that utilized Flysystem. The buggiest part of the initial implementation of Flysystem was the unnecessary methods such as dirname(), realpath(), and related methods that were also badly implemented. These have been refactored somewhat to make them better, but untangling the hacks that were put in place to force them to work is like trying to hook a race horse to a plow and expecting it to be effective :).
Flysystem defines protocols in configuration, which for example will allow for different definitions of a stream wrapper adapter that implement different protocols (i.e, "docx://", "pdf://", "image://" etc. I can then define media entities that are configured to utilize different stream wrapper adapters that will store an entry in the file managed table utilizing the protocol defined for the different types.
As a result, I can then send "docx://" to one S3 bucket, "pdf://" to a different S3 bucket, "image://" to Google Cloud Storage, and "styles://" to the local file system if my business use case necessitated such a structure. And I've used it to manage scientific studies that were PDFs living on different S3 buckets that were managed by the respective research teams in legacy systems outside of Drupal and uploaded to an S3 instance that they managed. We built a Drupal content management system to index the studies in Drupal and make them searchable with Solr. The team would then add the metadata about the study to the Drupal entry, and "publish" the study in Drupal, which would then in turn make it visible for Solr to index.
We used Flysystem to manage the Drupal integration with the different S3 buckets.
Can you provide the use case for this? Asking because of the number of failing tests on Drupal 10.1 and 10.2. Use cases would be helpful in determine if the tests are failing because of this patch, or for other reasons, as well as writing tests to cover this alteration.
@alexpott -- being able to return FALSE for dirname() would be awesome, especially for remote stream management to/from assets on object stores like S3. The original docblock notes indicated that both ::realpath() and ::dirname() werre supposed to be capable of returning a boolean, but the actual type hint on the return value as implemented were set for strings only.
(Begin my soapbox contribution)
IMO, we should not have either ::realpath() or ::dirname() on the StreamWrapperInterface, as the concept of both may not actually exist on the Streaming application that the wrapper is intended to integrate. For key/value object stores it's absolutely not applicable. If we ARE going to keep them on the StreamWrapperInterface, then we should make their implementation consistent with the PHP implementation of both:
https://www.php.net/manual/en/function.realpath (returns string or FALSE)
https://www.php.net/manual/en/function.dirname.php (returns string)
PHP's dirname() and realpath() don't support "streaming" per se. They both are implicitely structured to work with a traditional, local file system. Implementing dirname() and realpath() on Drupal\Core\File\FilesystemInterface is appropriate (somewhat), but I still think even there they should return FALSE when they're not applicable (like streaming applications).
- AWS PHP SDK StreamWrapper Interface: https://github.com/aws/aws-sdk-php/blob/master/src/S3/StreamWrapper.php <== neither method name exists
- Google Cloud Storage StreamWrapper class: https://github.com/googleapis/google-cloud-php-storage/blob/main/src/Str... <== neither method name exists
This ticket, and the ensuing discussion, exposes what is IMO a fundamental flaw in the way the Drupal Core has implemented StreamWrappers. The implementation has in implicit assumption that the underlying roots of any StreamWrapper implementation is working with a traditional mounted file system. In practice, these two methods cause workarounds that have to be implemented for a non-traditional alternative to the more traditional mounted file system, such as remote object stores.
As we have evolved other parts of Drupal Core to adapt to a constantly changing tech landscape, this is a particular component that needs to be revisited, as simple changes to the implementation open up a broader world of possibilities for more effectively using alternative file systems as they are intended to work. Breaking changes will likely result, but sometimes breaking changes are necessary to evolve.
A more radical change would be to decouple Drupal\Core\StreamWrapper\StreamWrapperInterface from Drupal\Core\StreamWrapper\PhpStreamWrapperInterface. This could actually be done without alot of disruption simply by:
- Making the two Wrapper Interfaces independent (in other words don't have StreamWrapperInterface implement PhpStreamWrapperInterface).
- Deprecate the "extended" methods on StreamWrapperInterface that are actually a part of PhpStreamWrapperinterface, with messages to revise any custom implementation (custom code and contrib modules) to implement BOTH interfaces.
- Pick a date for deprecations to take effect (11.x release perhaps)
- Revise Core to decouple the two interfaces
The code changes would be relatively minor, and this would make the StreamWrapper Management component of Drupal infinitely more flexible with regard to managing a wider variety of streaming services, including video streaming, podcasting, etc in addition to more traditional asset storage that is using non-traditional mounted file systems for storage (such as remote object stores, for example).
(End my soapbox contribution)
@kim.pepper -- I reviewed every defined in Drupal\Core\StreamWrapper\StreamWrapperInterface back in 2022 when I opened this issue. The two identified in this issue were the only ones whose documentation differed from the actual return type defined on the interface signature.
lhridley → created an issue.
lhridley → created an issue.
Closing as the 3.x-dev branch is deprecated in favor of the 3.0.x-dev branch.
lhridley → created an issue.
lhridley → created an issue.
lhridley → created an issue.
lhridley → created an issue.
This has been decoupled from the base League\Flysystem library, and will be pulled out of version 3.0.x of the Drupal module.
It will be implemented as a separate project, which will have a dependency on the D11/D10 (depending on timing of progress of the base refactor of the module). The main refactor issue can be followed:
https://www.drupal.org/project/flysystem/issues/31958 →
https://www.drupal.org/project/flysystem/issues/3378738
🌱
Planning for Release 3.0.0, Drupal 10.1+ Compatibility, Flysystem v3.1 Compatibility
Needs work
Please don't hesitate to reach out in the Drupal Slack channel for the Flysystem module, as I welcome the assistance in refactoring v2 of the module for Drupal 10+ support in v3+ of the module, which will upgrade implementation dependencies from version 1.x to version 3.x, and would love some assistance in getting there.
Amazon PHP SDK does support multipart uploads for large files: https://docs.aws.amazon.com/sdk-for-php/v3/developer-guide/s3-multipart-...
This module is currently in the process of a major refactoring effort to implement the Flysystem V3 Library, with the goal of releasing a Drupal 10 compatible version in the not-too-distant future. Since the current interation of the AWS SDK PHP library supports large file uploads by implementing async upload processes, this would be a welcome additon.
Please stay posted as the base refactoring progresses, because once that working model is ready for testing we'll move rapidly into implementing the adaptors for the different remote object services, the most important one will be the S3 service. Hopefully this will include support for large file uploads.
Follow along on progress for the refactoring effort at https://www.drupal.org/project/flysystem/issues/3378738 🌱 Planning for Release 3.0.0, Drupal 10.1+ Compatibility, Flysystem v3.1 Compatibility Needs work , and (main subissue) https://www.drupal.org/project/flysystem/issues/3195832 🌱 Implement 3.x version based on Flysystem v2 API changes Needs review .
lhridley → created an issue.
Closing this issue as it is no longer relevant due to the refactoring taking place on the current 3.0.x branch.
lhridley → created an issue.
lhridley → created an issue.
Updated the PR for the 10.3.x branch.
Traced documentation and implementation of dirname() on FileSystemInterface, while not directly related to StreamWrapperInterface, is nonetheless relevant.
All implementations of StreamWrapperInterface in core trace back to abstract class LocalStream, which handles (and can) returning a FALSE value on ::realpath(), so changing the documentation and return value typehinting on StreamWrapperInterface::realpath() to reflect a return value of FALSE.
Re-reviewed the implementation of FileSystemInterface::dirname() as well as PHP's dirname(), neither will return FALSE. LocalStream::dirname() also does not return FALSE as well. Revised the return value comment to remove reference to a FALSE return value.