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

Replace Zend_Service_Amazon_S3 with official AWS SDK. #17

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

colinmollenhour
Copy link

This isn't tested yet but would like to get your feedback.

Thanks!

@thaiphan
Copy link
Owner

thaiphan commented Dec 13, 2018

Hey @colinmollenhour,

I'm a big fan of your work!

I thought I wrote a reply a while ago but I guess I forgot to submit it!

Thanks for this pull request! However, I don't know if I can merge this. I've found that a lot of people still don't use Composer to manage their Magento dependencies and it will be a nightmare to tell them that the extension won't work. I've also been meaning to eventually upload this to the Magento Marketplace and I dunno if using Composer will work with that use-case.

What are your thoughts?

Regards,

Thai

@colinmollenhour
Copy link
Author

Thanks for the feedback.

For the marketplace you could package it up after running "composer install" locally so that the files are all included, but currently they still need to be include or linked up with the autoloader somehow. For github users that don't use composer you could add some additional installation steps such as "download AWS SDK here, extract, etc.." although I concede that is not ideal. Unfortunately since Magento 1 does not work with composer out of the box it is kinda messy, but I'm pretty sure you don't want to just include the entire Amazon SDK in your repo but that would be an option as well.

To avoid breaking existing installations you could merge the new version into a new branch such as "aws-sdk" (I'd recommend making this the new default branch at that point) which would be used for composer users and the packaged versions. Modman users using master and Composer users using "dev-master" would continue to get the old version so at least their installations wouldn't break. The new version could be a 2.x release so that 1.x remains on the Zend libs. Just some ideas.

There are some bugs with the code still so don't merge it, just wanted to get your feedback to see if this was a direction you were interested in taking it. If you are still interested I can push up the fixes so that the code works 100% but managing the branchs and release strategy will still be up to you. Let me know what you think and thanks for sharing your work!

@thaiphan
Copy link
Owner

thaiphan commented Feb 3, 2019

Hi @colinmollenhour,

Yeah, I am happy to maintain two separate branches:

  • master and develop will track the latest changes that you have made here and use the official AWS PHP SDK henceforth. Once we've merged your changes in, we will increment the semantic major version to 2.
  • 1.x will track the legacy code that uses the built-in Zend AWS library.

How do you feel about that?

Regards,

Thai

@colinmollenhour
Copy link
Author

Sounds good! Will update this branch with a more thoroughly tested PR in the next week or two.

… to check bucket access. Fix implementation bugs with official SDK migration. Add loaded_modules for proper config caching in get.php.
@colinmollenhour
Copy link
Author

Pushed another commit. Still not tested thoroughly yet but I think it's just about there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants