-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Upgrade flysystem to version ^2.0 #32534
Conversation
Hi @engcom-Kilo. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
84dc243
to
8013bea
Compare
@magento run all tests |
# Conflicts: # composer.lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request! Please see my comments
/** | ||
* Get file metadata. | ||
* | ||
* @deplacated There is no getMetadata() method in FilesystemAdapter anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to introduce this deplacated method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several clients for this method:
Magento\Eav\Model\Attribute\Data\Image, Magento\MediaGallerySynchronization\Model\CreateAssetFromFile, Magento\Framework\File\Mime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment and @deplacated
annotation should be removed.
Is implementing League\Flysystem\FilesystemAdapter
necessary?
/** | ||
* Remote storage cache interface. | ||
*/ | ||
interface CacheInterface extends FilesystemReader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't introduce extension points (interfaces) if that is not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved Reader interface methods to Cache interface. Removed inheritance
# Conflicts: # composer.lock
7a88b70
to
81fd686
Compare
@magento run all tests |
@@ -3,7 +3,8 @@ | |||
"description": "N/A", | |||
"require": { | |||
"php": "~7.3.0||~7.4.0", | |||
"magento/framework": "*" | |||
"magento/framework": "*", | |||
"predis/predis": "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"predis/predis": "*" | |
"predis/predis": "^1.1" |
@@ -14,8 +15,7 @@ | |||
"magento/module-media-storage": "*", | |||
"magento/module-import-export": "*", | |||
"magento/module-catalog-import-export": "*", | |||
"magento/module-downloadable-import-export": "*", | |||
"predis/predis": "*" | |||
"magento/module-downloadable-import-export": "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation should be updated as there is no need to require predis/predis
package https://devdocs.magento.com/guides/v2.4/config-guide/remote-storage/config-remote-storage-caching.html
/** | ||
* Remote storage cache interface. | ||
*/ | ||
interface CacheInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface should be removed. There is only one implementation and I wouldn't introduce a new extension point in the patch release
/** | ||
* Get file metadata. | ||
* | ||
* @deplacated There is no getMetadata() method in FilesystemAdapter anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment and @deplacated
annotation should be removed.
Is implementing League\Flysystem\FilesystemAdapter
necessary?
/** | ||
* Redis cache model. | ||
*/ | ||
class Predis implements CacheStorageHandlerInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced classes should be covered by integration tests
Flysystem is now updated in 2.4-develop |
Hi @engcom-Kilo, thank you for your contribution! |
Description (*)
Related Pull Requests
https://github.com/magento/partners-magento2ee/pull/530
Fixed Issues (if relevant)
Resolves #31177
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)