-
Notifications
You must be signed in to change notification settings - Fork 9.4k
[Backport] Remove unused namespace from Ui Export model #13 #15839
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
[Backport] Remove unused namespace from Ui Export model #13 #15839
Conversation
@mzeis Could you please check what cause failed travis? Thanks, |
@chirag-wagento It looks like this only was a temporary problem with the internet connection. I restarted the build and now it passes. |
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 current implementation (using WriteInterface)
is correct. As you see in https://github.com/chirag-wagento/magento2/blob/a61a3763fe7093e413c7fee54a3eb20bd20003c9/lib/internal/Magento/Framework/Filesystem.php#L78, Magento returns a WriterInterface
. What's actually passed might by a DirectoryList
class which implements WriterInterface
but it's best practice to use interfaces whenever possible.
@chirag-wagento Please have a look if you agree with my code review (I tend to reject this PR). Thanks! |
@mzeis |
@chirag-wagento Ok thanks! I will get feedback from the core team and inform you about what they say :) |
Any update in this PR ? Thanks |
@chirag-wagento Not yet, I am waiting for feedback. |
Hi @chirag-wagento! Unfortunately, I cannot accept your PR. #14834 was mistakenly accepted for Still, thank you for the time you have taken to create this PR! |
Original Pull Request
#14834
Description
The Export model have some unused namespaces:
Magento\Framework\Filesystem\Directory\WriteInterface
.Just removing it and update description
Manual testing scenarios
Contribution checklist