- πΊπΈUnited States lhridley
Switching this for 2.1.x branch, let's apply there and work toward a total replacement using the updated Flysystem V3 for 3.0.x
- Open on Drupal.org βCore: 10.0.7 + Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - @lhridley opened merge request.
- Open on Drupal.org βCore: 10.0.7 + Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago 103 pass - last update
over 1 year ago 103 pass - Open on Drupal.org βCore: 10.0.7 + Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago Composer error. Unable to continue. - last update
over 1 year ago 90 pass, 4 fail - last update
over 1 year ago 90 pass, 4 fail - Open on Drupal.org βCore: 10.0.7 + Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - Assigned to lhridley
- last update
over 1 year ago 103 pass - last update
over 1 year ago 103 pass - Open on Drupal.org βCore: 10.0.7 + Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - Open on Drupal.org βCore: 10.0.7 + Environment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago 103 pass - last update
over 1 year ago 103 pass - last update
over 1 year ago 103 pass - Issue was unassigned.
- Status changed to Needs review
over 1 year ago 10:28am 31 July 2023 - πΊπΈUnited States lhridley
Unfortunately just switching out the dependency is not an option without significant refactoring because ` m2mtech/flysystem-stream-wrapper` is a complete rewrite of `twistor/flysystem-stream-wrapper` with multiple change to the API.
Attached is a patch to port the code for the third party libraries that are dependencies for this module, effectively:
* twistor/flysystem-stream-wrapper
* league/flysystem
* league/flysystem-replicate-adapterAnd remediating the ported code for PHP 8.1 compliance and D10 compliance.
Would love to be able to go back to utilizing external libraries, as this adds alot of additional code to maintain, and I'm not yet convinced the test coverage is here to support the ported code (I did also port several of the tests, but may have missed some).
It's a good starting point.
- π§πͺBelgium dieterholvoet Brussels
I'm not sure copying the code of those libraries to this module is a good solution. We'll have to migrate to Flysystem V2 or V3 at some point, might as well do it now, right?
- πΊπΈUnited States lhridley
Oh, I agree, I'm looking to get the 2.1 branch stable for D10, then start working on the 3.0.x branch and using either Flysystem v2 or v3. That will be a breaking change because the APIs on V2 and V3 changed significantly from V1, with alot of refactoring along the way to eliminate the dependencies on abandoned third party packages.
- last update
over 1 year ago 103 pass - last update
over 1 year ago Composer require failure - πΊπΈUnited States lhridley
Ran a code coverage report after applying this patch:
* Code Coverage for the original module code (not ported) is very good, exceeding 90%.
More test coverage could be used:
Drupal\flysystem\EventSubscriber\EnsureSubscriber Methods: 66.67% ( 2/ 3) Lines: 92.31% ( 12/ 13) Drupal\flysystem\Asset\CssCollectionOptimizer Methods: 0.00% ( 0/ 1) Lines: 83.33% ( 10/ 12) Drupal\flysystem\Asset\JsCollectionOptimizer Methods: 0.00% ( 0/ 1) Lines: 83.33% ( 10/ 12)
For the Optimizers, the original maintainer extended Drupal Core classes instead of implementing Interfaces, so these statistics may be misleading, as test coverage may exist in Drupal Core for Methods that are part of the parent class.
At this time, I'm not going to port all of the tests for the ported code, primarily because the goal is to stabilize the module for D10, and focus on a rewrite for a new major version release using league/flysystem v2 or v3.
I'm going to merge this patch in "As Is" to the dev branch, but hold off for a bit on generating another release until this has had some time to percolate a bit.
- last update
over 1 year ago 103 pass -
lhridley β
committed cdc72cc4 on 2.1.x
Issue #3227240: Use a different package for stream wrapper support
-
lhridley β
committed cdc72cc4 on 2.1.x
- last update
over 1 year ago 103 pass - last update
over 1 year ago 103 pass - last update
over 1 year ago Build Successful - Status changed to Fixed
over 1 year ago 7:54am 1 August 2023 - π§πͺBelgium dieterholvoet Brussels
Are you sure all this is actually necessary? Because I have a project which is running Drupal 10.1.1, PHP 8.1 and Flysystem 2.1.0-rc4 and everything is working as expected. The only patches I have applied are these:
twistor/flysystem-stream-wrapper
flysystem
- π§πͺBelgium dieterholvoet Brussels
I think it would be enough to fix this on the 3.x branch.
- πΊπΈUnited States lhridley
Rethinking this one, which is why it's not yet part of a release.
- Status changed to Needs work
over 1 year ago 1:25pm 1 August 2023 - π§πͺBelgium dieterholvoet Brussels
I don't think this issue has anything to do with Drupal 10 / PHP 8.1 compatibility, as far as I know 2.1.0-rc5 is completely D10 compatible, except π Support new lazy asset collection optimizers & deprecate the serve_* options Needs review . This is just about getting rid of an abandoned dependency. Let's remove those tags and change this issue to target 3.x. If this does impact D10/PHP8.1 compatibility, please tell me how and maybe we can figure out another way to fix it.
- πΊπΈUnited States lhridley
I moved this back to "Needs Work", i have a revert patch running through tests currently, took a bit to make sure that I didn't lose other MRs that had been merged. I'll be updating the 2.1.x-dev branch with the revered code as soon as I get through with my team meetings today, in a couple of hours tops.
- b07e2fca committed on 2.1.x
Issue #3227240: Reverting the refactoring associated with eliminiating...
- b07e2fca committed on 2.1.x
- πΊπΈUnited States lhridley
Reverted the refactoring while keeping other merged MRs. Closing this issue.
- Status changed to Closed: outdated
over 1 year ago 2:22pm 1 August 2023 - π§πͺBelgium dieterholvoet Brussels
Why close? This still needs to happen in the 3.0.x branch, right?
- Status changed to Needs work
8 months ago 1:57pm 27 April 2024 - πΊπΈUnited States lhridley
Reopening this issue, as it is now relevant to the 3.0.x branch work. Will evaluate to determine what changes may be needed.