Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-1439: Upgraded codebase to rely on Flysystem v2 #171

Merged
merged 16 commits into from
Jan 13, 2023

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Nov 23, 2022

Question Answer
JIRA issue IBX-1439
Type feature
Target Ibexa version v4.4
BC breaks yes

This PR upgrades codebase to rely on Flysystem v2. We can't upgrade to the newest Flysystem v3 because of the hard requirement on PHP 8.

One of the most significant changes is the fact that Adapters were completely rewritten, requiring us to make significant changes to the code.

Our Flysystem Adapter implementation must support dynamic paths described by complex settings resolvable for the SiteAccess context. By default Local io.root_dir is defined as %webroot_dir%/$var_dir$/$storage_dir$ where var_dir and storage_dir are SiteAccess configuration parameters. Next to it there's the DFS (NFS) implementation which has one minor difference - web root is custom and static, defined via DFS_NFS_PATH environment variable. It could either be local path or a NFS mount which is transparently seen also as a local filesystem on all *nixes.

Per suggestion of Flysystem's Maintainer, Introducing DynamicPathFilesystemAdapterDecorator, proxying all calls to inner native Flysystem Local Adapter, after applying our dynamic path prefix using custom SiteAccess-aware PathPrefixer.
Additionaly to PathPrefixer, implementations of VisibilityConverter are provided, following Flysystem v2 \League\Flysystem\UnixVisibility\VisibilityConverter contracts. These resolve created file and directory permissions.

PathPrefixer has 2 implementations: LocalSiteAccessAwarePathPrefixer and DFSSiteAccessAwarePathPrefixer.
VisibilityConverter has 2 implementations: DFSVisibilityConverter and SiteAccessAwareVisibilityConverter. Please note that DFS doesn't include SiteAccessAware part in its name on purpose - DFS file/directory permissions are taken from ibexa.io.nfs.adapter.config Container parameters and are not dynamic.

The structure of the implementation could be presented on the following diagram:

classDiagram
direction BT
class FilesystemAdapter
class DynamicPathFilesystemAdapterDecorator
class PathPrefixerInterface
class BaseSiteAccessAwarePathPrefixer
class DFSSiteAccessAwarePathPrefixer
class BaseVisibilityConverter
class DFSVisibilityConverter
class VisibilityConverter


BaseSiteAccessAwarePathPrefixer  ..>  PathPrefixerInterface
DFSSiteAccessAwarePathPrefixer  -->  BaseSiteAccessAwarePathPrefixer  
LocalSiteAccessAwarePathPrefixer  -->  BaseSiteAccessAwarePathPrefixer

BaseVisibilityConverter  ..>  VisibilityConverter
DFSVisibilityConverter  -->  BaseVisibilityConverter
SiteAccessAwareVisibilityConverter  -->  BaseVisibilityConverter

DynamicPathFilesystemAdapterDecorator  ..>  FilesystemAdapter
DynamicPathFilesystemAdapterDecorator *-- FilesystemAdapter
DynamicPathFilesystemAdapterDecorator *-- PathPrefixerInterface
FilesystemAdapter *-- BaseVisibilityConverter
Loading

Features no longer supported

  1. Support for overwriting existing files has been dropped (catch block of \Ibexa\Core\IO\IOBinarydataHandler\Flysystem::create + test) as now native Flysystem Local Adapter does this OOTB.
  2. Support (in the form of test case) for no last modified timestamp (allegedly unset by AWS/S3 handler in the past) has been dropped as right now Flysystem throws UnableToRetrieveMetadata exception in such case.

Documentation

Upgrade instruction

Local adapters' (by default defined in config/packages/oneup_flysystem.yaml) directory key becomes location

E.g.:

Before

oneup_flysystem:
     adapters:
         default_adapter:
             local:
                directory: '%kernel.cache_dir%/flysystem'

After

oneup_flysystem:
    adapters:
        default_adapter:
            local:
                location: '%kernel.cache_dir%/flysystem'

If you haven't touched these files, you can simply reset 3rd party oneup/flysystem-bundle recipe by executing:

composer recipe:install --force --reset -- oneup/flysystem-bundle

Backward compatibility breaks

Requiring higher major version in a minor release is a necessary BC break. We are forced to do that because it seems 1.x is no longer maintained and moreover 3rd party adapters rely higher Flysystem version.
If a custom project relies directly on Flysystem features instead of using our IO abstraction, it will require an upgrade as well, using the following instructions: https://flysystem.thephpleague.com/docs/upgrade-from-1.x/

Other docs

Page https://doc.ibexa.co/en/latest/infrastructure_and_maintenance/clustering/clustering_with_aws_s3/#set-up-aws-s3-account
needs to be updated:

composer require league/flysystem-aws-s3-v3:^2.0

QA

Affected scopes / features:

  • NFS / DFS adapters
  • IBX-3740 regarding DFS_NFS_PATH configuration
  • Multirepository storage var_dir for files, images, etc - when directory is different per Repository's SiteAccesses
  • Content preview which emits \Ibexa\Core\MVC\Symfony\Event\ScopeChangeEvent affecting path prefixes as well
  • CLI: Symfony command using --siteaccess option.
  • CLI: Symfony command ibexa:images:normalize-paths
  • Local adapters now by default should overwrite existing files - our support for that has been dropped, delegating it to Flysystem
  • Platform.Sh NFS & local (or do we use NFS-only there?) binary files operations.

Tasks

  • Bump Flysystem dependencies to rely on Flysystem 2.x
  • Upgrade codebase to use Flysystem 2.x
  • Investigate OneUp/FlysystemBundle requirements
  • Confirm that Flysystem v2 by default overwrites existing file
  • Fix failing tests
  • Upgrade other packages with root dependencies on Flysystem or OneUp Flysystem Bundle
  • Upgrade other packages with dev dependency on Flysystem Memory adapter

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly.
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review.

@alongosz alongosz added Doc needed The changes require some documentation Feature New feature request Work in progress labels Nov 23, 2022
@alongosz alongosz force-pushed the ibx-1439-upgrade-flysystem branch 4 times, most recently from eaed080 to 8660cf2 Compare December 13, 2022 11:21
@alongosz alongosz marked this pull request as ready for review December 13, 2022 11:52
@alongosz alongosz requested a review from a team December 13, 2022 13:59
Copy link
Contributor

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet I think, that at least a question (as contribution probably would require too much work) to Flysystem should be sent anyway (if not the case already) to avoid maintaining overridden code.

src/lib/IO/IOMetadataHandler/Flysystem.php Outdated Show resolved Hide resolved
@konradoboza konradoboza requested a review from a team December 14, 2022 14:45
@alongosz
Copy link
Member Author

Update: b280ad8 fixed outstanding legacy test parameter values for storage root dir.

@alongosz
Copy link
Member Author

alongosz commented Jan 3, 2023

Moving this to draft state as part of the solution needs to be changed according to the feedback in thephpleague/flysystem#1614

@alongosz alongosz marked this pull request as draft January 3, 2023 22:36
alongosz and others added 15 commits January 5, 2023 14:01
oneup/flysystem-bundle ^4.4.2 requires league/flysystem ^2.0 || ^3.0
Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>
Co-Authored-By: Mikolaj Adamczyk <mikadamczyk@users.noreply.github.com>
Co-Authored-By: Mikolaj Adamczyk <mikadamczyk@users.noreply.github.com>
Co-Authored-By: Paweł Niedzielski <Steveb-p@users.noreply.github.com>
Co-Authored-By: Adam Wójs <adamwojs@users.noreply.github.com>
Co-Authored-By: Adam Wójs <adamwojs@users.noreply.github.com>
Co-Authored-By: Paweł Niedzielski <Steveb-p@users.noreply.github.com>
Co-Authored-By: Paweł Niedzielski <Steveb-p@users.noreply.github.com>
@webhdx
Copy link
Contributor

webhdx commented Jan 5, 2023

I rebased it against main so we can run regression tests again.

@alongosz alongosz marked this pull request as ready for review January 10, 2023 14:01
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA approved on IbexaDXP commerce 4.4.x.

@alongosz alongosz merged commit 7d76881 into main Jan 13, 2023
@alongosz alongosz deleted the ibx-1439-upgrade-flysystem branch January 13, 2023 10:16
alongosz added a commit to ibexa/user that referenced this pull request Jan 13, 2023
For more details see https://issues.ibexa.co/browse/IBX-1439 and ibexa/core#171

The dependency is taken from ibexa/core which contains code requiring it.
alongosz added a commit to ibexa/solr that referenced this pull request Jan 13, 2023
For more details see https://issues.ibexa.co/browse/IBX-1439 and ibexa/core#171

The dependency is taken from ibexa/core which contains code requiring it.
@MagdalenaZuba MagdalenaZuba removed the Doc needed The changes require some documentation label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.