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

NEXT-17739 Catch GCP NotFoundException when copying assets #2108

Closed
wants to merge 3 commits into from
Closed

NEXT-17739 Catch GCP NotFoundException when copying assets #2108

wants to merge 3 commits into from

Conversation

eXistenZNL
Copy link

@eXistenZNL eXistenZNL commented Oct 6, 2021

1. Why is this change necessary?

Having this fix makes is possible to install Shopware in an environment that uses a Google Cloud Platform bucket as the public filesystem.

2. What does this change do, exactly?

It catches the thrown Google\Cloud\Core\Exception\NotFoundException upon copying the assets for bundles to Google Cloud. The reason this exception is thrown is that the export action always removes any pre-existing assets and copies a fresh set of bundle assets in place. However if the directory does not exist yet on Google Cloud bucket, the exception is thrown by the Google Cloud PHP library that is used by the superbalist/flysystem-google-cloud-storage package.

3. Describe each step to reproduce the issue or behaviour.

  1. Configure Shopware6 to use a public filesystem of type google-storage.
  2. Do a complete Shopware6 installation from scratch by running bin/console system:install.
  3. See that the installer breaks when copying the assets because of the exception thrown.

4. Please link to the relevant issues (if any).

https://issues.shopware.com/issues/NEXT-17739

5. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

@CLAassistant
Copy link

CLAassistant commented Oct 6, 2021

CLA assistant check
All committers have signed the CLA.

@eXistenZNL eXistenZNL changed the title Next 17739 catch gcp not found exception when copying assets NEXT-17739 Catch GCP NotFoundException when copying assets Oct 6, 2021
$this->filesystem->deleteDir($targetDirectory);

try {
$this->filesystem->deleteDir($targetDirectory);
Copy link
Member

Choose a reason for hiding this comment

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

implement own filesystem adapter which is extending from the GCP and catch that error there. On the application level, we should not catch specific filesystem exceptions

Copy link
Author

Choose a reason for hiding this comment

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

That's fair. I will create an MR for superbalist/flysystem-google-cloud-storage so it catches the exception there which makes Shopware work as intended 👍

Copy link
Author

@eXistenZNL eXistenZNL Oct 6, 2021

Choose a reason for hiding this comment

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

Upstream patch created: Superbalist/flysystem-google-cloud-storage#122.

Please advice on what to do next, will any new release of https://github.com/Superbalist/flysystem-google-cloud-storage be picked up automatically somewhere during the preparation of a new Shopware 6 release (as long as the Flysystem GCP package release is following SemVer), or do I need to do a manual action for this?

Copy link
Member

Choose a reason for hiding this comment

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

currently a specific version of this package is pinned in our composer.json

so this needs an adjustment, once your upstream fix was released

Copy link
Member

Choose a reason for hiding this comment

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

Could you change in the composer.json the pinning in this pull request? So when upstream releases a new version, it will be directly picked up?

Copy link
Author

Choose a reason for hiding this comment

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

Yes I will! this MR is closed for now, which is fine, will bump it when merged upstream!

@shyim
Copy link
Member

shyim commented Oct 20, 2021

I am unpinning the dependency, so it will be updated when they release it. Should be merged the next days. Anyway thanks! 😄

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.

4 participants